array cookie support #73

alexanderlinne · 2021-05-10T15:33:20Z

closes #71

Instead of collecting all bitcasts and filtering them, we observe that in case a GEP instruction is applied to an allocation it usually is used to calculate the pointer of the actual array. In case two GEP instructions are present, the first is used to add necessary padding to the array cookie and it's bitcast instructions can be safely ignored.

Introduces a new data structure for array cookie information that stores the store instruction that writes to the cookie and the value of the padding passed to the GEP instruction that computes the array pointer passed to the user. This commit also introduces several asserts to catch cases which are not expected and should be further investigated.

This makes these two command line arguments available in both the MemInstFinderPass and TypeARTPass. This allows the MemInstFinder to only collect the necessary instructions. As heap allocations are only collected on unoptimized code this in turn allows for a stricter analysis of the given code. To share the command line arguments, the cl::opt instances have been shared between the two passes. As the TypeARTPass depends on the MemInstFinderPass to already be loaded, the instances have been moved to the MemInstFinderPass and are now referenced with external linkage in the TypeARTPass.

If an array cookie is present, the value of the arrary cookie store instruction will be used as the element count of the array.

coveralls · 2021-05-10T15:57:52Z

Pull Request Test Coverage Report for Build 853856281

105 of 112 (93.75%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.002%) to 89.437%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/passes/support/Error.h	0	2	0.0%
lib/passes/analysis/MemOpVisitor.cpp	77	82	93.9%

Files with Coverage Reduction	New Missed Lines	%
lib/passes/analysis/MemOpVisitor.cpp	1	95.95%

Totals
Change from base Build 852903693:	-0.002%
Covered Lines:	2921
Relevant Lines:	3266

💛 - Coveralls

In case of array cookies, TypeART previously instrumented the address of the full memory block including the cookie. With this commit this is changed s.t. the memory address of the actual array is instrumented.

lib/passes/analysis/MemOpData.h

lib/passes/analysis/MemOpVisitor.cpp

lib/passes/instrumentation/MemOpInstrumentation.cpp

lib/passes/analysis/MemOpVisitor.cpp

ahueck

I have added some minor comments/changes. Overall great patch.

Eventually, we should do a rebase of the branch w.r.t. new changes in devel and then I will squash merge.

ahueck · 2021-05-11T16:06:26Z

Seems like the assert triggers in expectSingleUser when compiled with ASAN/UBSAN for the runtime test 41_array_cookie.cpp.

apply.sh 41_array_cookie.cpp 2>&1
opt: /home/ahueck/workspace/typeart/lib/passes/analysis/MemOpVisitor.cpp:79: T *typeart::finder::expectSingleUser(llvm::Value *) [T = llvm::StoreInst]: Assertion `users_it == users.end()' failed.

 #9 0x00007f2c8f9b8747 llvm::StoreInst* typeart::finder::expectSingleUser<llvm::StoreInst>(llvm::Value*) /home/ahueck/workspace/typeart/lib/passes/analysis/MemOpVisitor.cpp:80:3
#10 0x00007f2c8f9b2363 typeart::finder::handleUnpaddedArrayCookie(llvm::SmallPtrSet<llvm::GetElementPtrInst*, 2u> const&, llvm::SmallPtrSet<llvm::BitCastInst*, 4u>&, llvm::BitCastInst*&) /home/ahueck/workspace/typeart/lib/passes/analysis/MemOpVisitor.cpp:124:8
#11 0x00007f2c8f9b28a1 typeart::finder::handleGepInstrs(llvm::SmallPtrSet<llvm::GetElementPtrInst*, 2u> const&, llvm::SmallPtrSet<llvm::BitCastInst*, 4u>&, llvm::BitCastInst*&) /home/ahueck/workspace/typeart/lib/passes/analysis/MemOpVisitor.cpp:160:12

When ASAN is enabled, it will track array cookies using instrumentation by passing the array cookie-related bitcast. This triggered an assert of TypeART in "expectSingleUser", as we expected only a single user of the bitcast (the store of the array length) This patch explicitly handles the ASAN cookie instrumentation.

ahueck · 2021-05-11T17:59:49Z

@alexanderlinne I fixed the ASAN issue w.r.t. expectSingleUser and renamed the function.
Not sure if its a better name.

alexanderlinne · 2021-05-11T18:32:24Z

@ahueck "get" implies that this function behaves like a normal getter, i.e. I would expect it to not assert. I usually use assert... for void function that perform assertions and expect... for such functions with a return value.
LOG_FATAL should probably just be an assert as well. Otherwise all callees would have to test for nullptr and handle them. Also, the error here, I think, is not that we couln't find a non-asan user, but that we couldn't find a user of the desired type.

use llvm's Expected type to handle errors using some macros that simplify control flow

* return_error_if merged to always use formatv version * typeart::error namespace lowercase * some minor code format

alexanderlinne added 10 commits May 7, 2021 14:50

refactor MemOpVisitor::visitMallocLike

020c81d

add tests for padded and unpadded array cookies

65684d7

add array cookie handling to the instrumentation

0817a3a

If an array cookie is present, the value of the arrary cookie store instruction will be used as the element count of the array.

fix tests

08254d8

add test for array cookies of dynamically sized arrays

22d8b4f

remove wrong comments from array cookie tests

eabaac9

fix formatting

2931463

alexanderlinne requested a review from ahueck May 10, 2021 15:41

alexanderlinne and others added 2 commits May 11, 2021 16:35

instrument actual array address

650b1a3

In case of array cookies, TypeART previously instrumented the address of the full memory block including the cookie. With this commit this is changed s.t. the memory address of the actual array is instrumented.

add runtime array cookie test

69dfe8d