Overall, this looks fine. It won't break anything to land this in google3. http://codereview.appspot.com/4867059/diff/5001/lib/Transforms/Instrumentation/AddressSanitizer.cpp File ...
12 years, 8 months ago
(2011-08-18 22:37:10 UTC)
#2
> > http://codereview.appspot.com/4867059/diff/5001/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode486 > lib/Transforms/Instrumentation/AddressSanitizer.cpp:486: MappingOffsetLog = > LongSize == 32 > How about testing ...
12 years, 8 months ago
(2011-08-18 23:01:09 UTC)
#3
>
>
http://codereview.appspot.com/4867059/diff/5001/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:486: MappingOffsetLog =
> LongSize == 32
> How about testing for LongSize == 32 ... LongSize == 64 ... else return false?
Added assert(LongSize == 32 || LongSize == 64);
asan should not be invoked on configurations where this does not hold.
>
>
http://codereview.appspot.com/4867059/diff/5001/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:524: // Workaround for a
> strange compile assertion while building 483.xalancbmk
> Ah. For the same reason, you will want to return true when the BB has a
> LandingPadInst. It'll be the first instr right after the PHI nodes (you can
use
> BB->getFirstNonPHI()).
This was enough. I'd prefer to leave it as is (they say EH is going to be redone
any way)
===
Fixed all others.
12 years, 8 months ago
(2011-08-23 00:12:19 UTC)
#5
On 2011/08/22 23:12:11, nlewycky1 wrote:
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right):
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:70: #include <stdint.h>
> Please use #include "llvm/Support/DataTypes.h" instead.
done
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:71: #include <stdio.h>
> Why? What are you using out of this?
removed
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:128: static const char
> *kAsanModuleCtorName = "asan.module_ctor";
> Being static, this doesn't need to be inside the anonymous namespace.
done
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:264: if (LoadInst *ld =
> dyn_cast<LoadInst>(inst)) {
> LLVM's common naming would make this:
> if (LoadInst *LI = dyn_cast<LoadInst>(I)) {
done
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:303: std::string
> FunctionName = "__asan_report_error_" + itostr(TelltaleValue);
> Why isn't the TelltaleValue just a ConstantInt parameter to a single
> __asan_report_error function?
to save code size a bit. Added a comment.
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:306: CallInst *call =
> IRB.CreateCall(AsanReportWarning, Addr);
> __asan_report_error always terminates, right?
Yes, although this may not be true in the future.
> Then you can add
> "call->setDoesNotReturn();". That should help out codegen a bit.
I tried that with the ud2 code and codegen crashed on one of the CPU2006
benchmarks.
I'd prefer not to use setDoesNotReturn for now, but will likely want to try it
later (it does save some code size).
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:358: unsigned
> log_of_size_in_bytes = __builtin_ctz(TypeSize / 8);
> Not all compilers that build LLVM support __builtin_ctz. Please use
> CountTrailingZeros_32 (or _64) from ADT/MathExtras.h.
done
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:425: index < n; ++index) {
> LLVM always uses "index != n" for loops like this.
done
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:440: CurrentCtors.size());
> This should line up with the "(R"
done
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:463: LongTy =
> Type::getIntNTy(*C, LongSize);
> This is intptr_t, right? How about IntPtrTy? It's a little confusing to see
> references to LongTy in pointer casts everywhere.
done
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:483: assert(LongSize == 32
> || LongSize == 64);
> How about we just "return false;" in this case?
done
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:496:
> "__asan_mapping_offset");
> What if I hand you a module where this value already exists? Currently, it
will
> create "__asan_mapping_offset2".
Ouch
> Make sure that your code also doesn't crash if
> you're asked to run on a Module that *has* __asan_mapping_offset, but it has
the
> wrong type. Same with the other __asan_... variables and functions.
Something bad will happen.
Can we just assert that it won't happen?
Names with __ prefix are not allowed in user code anyway (afaict)
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:515: return res;
> The return value should control whether or not a change to the module was
made.
> Of course, this pass always changes the module.
>
> I have an idea: the per-Function part is entirely parallelizable, so turn this
> from a ModulePass into a FunctionPass, and turn handleFunction into
> runOnFunction. Move F->isDeclaration() into runOnFunction.
I tried making it a FunctionPass and it did not work (sadly, I don't remember
why).
Can this change go as a separate improvement if really needed?
In the original file I also insert redzones between globals and this can not be
done by a FunctionPass, right?
>
> These other parts which add __asan_... (don't forget __asan_report_error_*)
and
> modify llvm.global_ctors should move into doInitialization(Module &).
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:529: if
(isa<CallInst>(BI))
> {
> This test is redundant, you just need the dyn_cast below.
done
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:540: static bool
> blockOrItsSuccHasException(BasicBlock &bb) {
> bb -> BB
done
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:561: for
(Function::iterator
> FI = F.begin(), FE = F.end();
> I think you can fuse these two loops and remove the InstructionsToInstrument
> set.
>
> Create "SmallVector<BasicBlock *, 16> Worklist" to use as a queue and only put
> F.getEntryBlock() in it, then enter a "while (!Worklist.empty())" loop which
> inserts the successor BBs into the Worklist before instrumenting all the
> load/store/memintrin instructions. The instrumentation may add new BBs that
you
> don't want to visit, and because you didn't add those BBs to your worklist,
you
> won't.
These two loops are deliberate choice, although it is not evident from the code
and comments.
I added a comment:
// At this point InstructionsToInstrument contains a set of instructions
// that need to be instrumented. However, instrumenting some of them
// might be proved redundant. In future we may add more analysis
// to avoid redundant instrumentation. See
// http://code.google.com/p/address-sanitizer/wiki/CompileTimeOptimizations
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:603: NumInstrumented++;
> I don't think this logic is quite right; this will fire even when no changes
are
> made, causing runOnModule to return true on an unmodified module. Also, why
> isn't it a bool?
NumInstrumented counts how many insns have been instrumented.
It also enables me to bisect a particular insn that causes a problem if
instrumented (ClDebugMin/Max).
Btw, does LLVM has a debug API for bisecting?
W/o the debug flags NumInstrumented is incremented only if something was
instrumented.
>
>
http://codereview.appspot.com/4867059/diff/6002/lib/Transforms/Instrumentatio...
> lib/Transforms/Instrumentation/AddressSanitizer.cpp:611: errs() << F;
> There's a dbgs() method you can use, which returns wither a void sync, or
> errs(). It's controlled by whether -debug-pass=asan is enabled (see the
> DEBUG_TYPE macro you set at the top). For that matter, please feel free to
> sprinkle dbgs() << ... around your pass.
done.
Issue 4867059: AddressSanitizer/LLVM first patch
Created 12 years, 8 months ago by kcc2
Modified 8 years, 7 months ago
Reviewers: nlewycky1
Base URL: http://llvm.org/svn/llvm-project/llvm/trunk/
Comments: 36
