[ASAN] Insert call to __asan_init and load of dynamic shadow address in correct order

[evgeny777]

Summary

Good time of the day!

I've ported ASAN to proprietary system, where I (besides everything else) need to instrument application startup code. To accomplish this task I've added check for appropriate function name and triple in maybeInsertAsanInitAtFunctionEntry(). However I got following IR after that:

define void @_startup() #0 {
entry:
  %0 = load i64, i64* @__asan_shadow_memory_dynamic_address
  call void @__asan_init()
  .....

Needless to say that it doesn't work as expected, because asan_shadow_memory_dynamic_address is initialized in asan_init().
This patch fixes the problem for me, by enforcing correct order of operations.
I don't know how to write unit test for it, so any suggestions/comments are appreciated.

Thanks.

[kcc]

I don't think this is the right fix.
Please explain more: why does this load from __asan_shadow_memory_dynamic_address happen in a function that does not have sanitize_address attribute?

I don't know how to write unit test for it, so any suggestions/comments are appreciated.

Check the test/Instrumentation/AddressSanitizer/ dir.
Maybe, a proper test will help explain the fix.

[evgeny777]

Thanks for looking at it.

Let's assume we have function which *does* have sanitize_address attribute. Now imagine we have function for which both maybeInsertAsanInitAtFunctionEntry and maybeInsertDynamicShadowAtFunctionEntry return true.
If so then:

• Line 2250 (maybeInsertAsanInitAtFunctionEntry) will insert call to __asan_init at the top of our function
• Then Line 2263 (maybeInsertDynamicShadowAtFunctionEntry) will insert load of dynamic shadow address at the top of our function (before __asan_init)

We'll get both things inserted in reverse order (and undefined behavior when running app).

[kcc]

Ok.. makes sense. These did not collide before because maybeInsertDynamicShadowAtFunctionEntry is mostly used on WIndows and maybeInsertAsanInitAtFunctionEntry only on Mac. grrr.
Now it does make sense, but I would prefer if the code did not have a duplicated call to maybeInsertAsanInitAtFunctionEntry
(and, of course, a test)

lib/Transforms/Instrumentation/AddressSanitizer.cpp

void AddressSanitizer::markEscapedLocalAllocas(Function &F) {
   }
 }
 
 bool AddressSanitizer::runOnFunction(Function &F) {
   if (F.getLinkage() == GlobalValue::AvailableExternallyLinkage) return false;
   if (!ClDebugFunc.empty() && ClDebugFunc == F.getName()) return false;
   if (F.getName().startswith("__asan_")) return false;
 
-  bool FunctionModified = false;
-
-  // If needed, insert __asan_init before checking for SanitizeAddress attr.
-  // This function needs to be called even if the function body is not
-  // instrumented.  
-  if (maybeInsertAsanInitAtFunctionEntry(F))
-    FunctionModified = true;
-  
   // Leave if the function doesn't need instrumentation.
-  if (!F.hasFnAttribute(Attribute::SanitizeAddress)) return FunctionModified;
+  // The __asan_init function needs to be called even if the function body is
+  // not instrumented.
+  if (!F.hasFnAttribute(Attribute::SanitizeAddress))
+    return maybeInsertAsanInitAtFunctionEntry(F);
 
   DEBUG(dbgs() << "ASAN instrumenting:\n" << F << "\n");
 
   initializeCallbacks(*F.getParent());
   DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
 
   FunctionStateRAII CleanupObj(this);
 
   maybeInsertDynamicShadowAtFunctionEntry(F);
+  bool FunctionModified = maybeInsertAsanInitAtFunctionEntry(F);
 
   // We can't instrument allocas used with llvm.localescape. Only static allocas
   // can be passed to that intrinsic.
   markEscapedLocalAllocas(F);
 
   // We want to instrument every address only once per basic block (unless there
   // are calls between uses).
   SmallSet<Value *, 16> TempsToInstrument;