Issue 6432065: ASan Initialization Order Checking - LLVM half
DescriptionInitialization order checking for ASan - LLVM half.
Patch Set 1 #
Total comments: 9
Patch Set 2 : Second version #Patch Set 3 : Third Version (uploaded wrong patch) #
Total comments: 2
Patch Set 4 : Minor changes #
Total comments: 10
Patch Set 5 : More changes #
Total comments: 2
Patch Set 6 : Even More changes #
Total comments: 8
MessagesTotal messages: 15
http://codereview.appspot.com/6432065/diff/1/lib/Transforms/Instrumentation/A... File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right): http://codereview.appspot.com/6432065/diff/1/lib/Transforms/Instrumentation/A... lib/Transforms/Instrumentation/AddressSanitizer.cpp:383: UE != UI; ++UI) { one more leading space here? http://codereview.appspot.com/6432065/diff/1/lib/Transforms/Instrumentation/A... lib/Transforms/Instrumentation/AddressSanitizer.cpp:389: if (CurrentFunction->getName().find("__cxx_global_var_init") == 0) { Long chain of -> looks a bit scary. Are there really guarantees that CurrentFunction or CurrentFunction->getName() can't be null? http://codereview.appspot.com/6432065/diff/1/lib/Transforms/Instrumentation/A... lib/Transforms/Instrumentation/AddressSanitizer.cpp:409: if (HasDynamicInitializer(G)) Just curious - will that be slow, if there are many stores to a global variable? You iterate over all uses for every mop, right?
Sign in to reply to this message.
http://codereview.appspot.com/6432065/diff/1/lib/Transforms/Instrumentation/A... File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right): http://codereview.appspot.com/6432065/diff/1/lib/Transforms/Instrumentation/A... lib/Transforms/Instrumentation/AddressSanitizer.cpp:381: static bool HasDynamicInitializer(GlobalVariable *G) { Before we enable this phase by default, this function should be re-implemented using info from clang. Please add a FIXME comment. http://codereview.appspot.com/6432065/diff/1/lib/Transforms/Instrumentation/A... lib/Transforms/Instrumentation/AddressSanitizer.cpp:409: if (HasDynamicInitializer(G)) Shouldn't the logic be opposite? We want to skip accesses to linker-initialized globals. Also, Alexey is right. Calling this expensive function on every access is slow. For now, please do it under ClInitializers http://codereview.appspot.com/6432065/diff/1/lib/Transforms/Instrumentation/A... lib/Transforms/Instrumentation/AddressSanitizer.cpp:670: bool PoisonDuringInitialization = HasDynamicInitializer(G); prefer the name like GlobalHasDynamicInitializer
Sign in to reply to this message.
Second version
Sign in to reply to this message.
See message on the compiler-rt review Much appreciated, Reid http://codereview.appspot.com/6432065/diff/1/lib/Transforms/Instrumentation/A... File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right): http://codereview.appspot.com/6432065/diff/1/lib/Transforms/Instrumentation/A... lib/Transforms/Instrumentation/AddressSanitizer.cpp:381: static bool HasDynamicInitializer(GlobalVariable *G) { On 2012/07/24 08:11:49, kcc1 wrote: > Before we enable this phase by default, > this function should be re-implemented using info from clang. > Please add a FIXME comment. I modified clang and this method to use metadata. See patch set 2 http://codereview.appspot.com/6432065/diff/1/lib/Transforms/Instrumentation/A... lib/Transforms/Instrumentation/AddressSanitizer.cpp:409: if (HasDynamicInitializer(G)) On 2012/07/24 08:11:49, kcc1 wrote: > Shouldn't the logic be opposite? > We want to skip accesses to linker-initialized globals. > > Also, Alexey is right. Calling this expensive function on every access is slow. > For now, please do it under ClInitializers I've added in a check for ClInitializers, and fixed up the logic. http://codereview.appspot.com/6432065/diff/1/lib/Transforms/Instrumentation/A... lib/Transforms/Instrumentation/AddressSanitizer.cpp:670: bool PoisonDuringInitialization = HasDynamicInitializer(G); On 2012/07/24 08:11:49, kcc1 wrote: > prefer the name like GlobalHasDynamicInitializer Done
Sign in to reply to this message.
http://codereview.appspot.com/6432065/diff/10001/lib/Transforms/Instrumentati... File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right): http://codereview.appspot.com/6432065/diff/10001/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:387: for (int i = 0, n = DynamicGlobals->getNumOperands(); Instead of traversing the metadata on every call, please instead create a ValueMap of dynamically initialized globals (or some other map; put it into the AddressSanitizer class) and then simply query it. http://codereview.appspot.com/6432065/diff/10001/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:704: if (GlobalHasDynamicInitializer) { no need for { and }
Sign in to reply to this message.
Minor changes
Sign in to reply to this message.
Thanks! I'll try to take a careful look at these patches tomorrow. On Tue, Jul 31, 2012 at 5:32 AM, <reidw@google.com> wrote: > Minor changes > > http://codereview.appspot.com/**6432065/<http://codereview.appspot.com/6432065/> > -- Alexey Samsonov, MSK
Sign in to reply to this message.
http://codereview.appspot.com/6432065/diff/14001/lib/Transforms/Instrumentati... File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right): http://codereview.appspot.com/6432065/diff/14001/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:386: = M.getNamedMetadata("dynamically_initialized_globals"); Nit: please move "=" to the previous line, or use if (NamedMDNode *DynamicGlobals = M.getNamedMetadata(...)) { ... } instead http://codereview.appspot.com/6432065/diff/14001/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:390: i < n; ++i) { Nit: move it to prev line http://codereview.appspot.com/6432065/diff/14001/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:414: // dynamically initialized global is always valid. Just a random thought: Are access to globals always valid if you instrument Mop which doesn't happen inside initializers? If yes, do you think you can obtain this data from smth like function context? http://codereview.appspot.com/6432065/diff/14001/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:420: if (G->getLinkage() != GlobalVariable::ExternalLinkage && It looks like you need || instead of && here. http://codereview.appspot.com/6432065/diff/14001/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:736: bool ModuleHasDynamicInitializers = M.getFunction("_GLOBAL__I_a"); You can pass fetched _GLOBAL__I_a to createInitializerPoisonCalls.
Sign in to reply to this message.
See comments on compilerrt review. http://codereview.appspot.com/6432065/diff/14001/lib/Transforms/Instrumentati... File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right): http://codereview.appspot.com/6432065/diff/14001/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:386: = M.getNamedMetadata("dynamically_initialized_globals"); On 2012/08/01 16:27:10, samsonov wrote: > Nit: please move "=" to the previous line, or use > if (NamedMDNode *DynamicGlobals = M.getNamedMetadata(...)) { > ... > } > instead Done. http://codereview.appspot.com/6432065/diff/14001/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:390: i < n; ++i) { On 2012/08/01 16:27:10, samsonov wrote: > Nit: move it to prev line Done http://codereview.appspot.com/6432065/diff/14001/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:414: // dynamically initialized global is always valid. On 2012/08/01 16:27:10, samsonov wrote: > Just a random thought: > > Are access to globals always valid if you instrument Mop which doesn't happen > inside initializers? If yes, do you think you can obtain this data from smth > like function context? I'm not sure I follow. Are you referring to skipping instrumentation of loads/stores inside of the actual compiler-generated initializer? Or the function called by the user to initialize a given variable? http://codereview.appspot.com/6432065/diff/14001/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:420: if (G->getLinkage() != GlobalVariable::ExternalLinkage && On 2012/08/01 16:27:10, samsonov wrote: > It looks like you need || instead of && here. The comment could have been written better (changed in next patch). However, we still need to instrument access to global variables which do not have dynamic initializers in this TU, but have external linkage. For example, extern int a; would result in this TU seeing a global variable which seemed to not have an initializer (in the current TU), but which could have dynamic initialization in the TU it was defined in. http://codereview.appspot.com/6432065/diff/14001/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:736: bool ModuleHasDynamicInitializers = M.getFunction("_GLOBAL__I_a"); On 2012/08/01 16:27:10, samsonov wrote: > You can pass fetched _GLOBAL__I_a to createInitializerPoisonCalls. Thanks for pointing this out -- this is extraneous.
Sign in to reply to this message.
More Changes
Sign in to reply to this message.
This also looks fine. http://codereview.appspot.com/6432065/diff/16002/lib/Transforms/Instrumentati... File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right): http://codereview.appspot.com/6432065/diff/16002/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:421: // in a different TU. Oh, got it. Thanks for clarification. http://codereview.appspot.com/6432065/diff/16002/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:733: ArrayType *ArrayOfDynamicInitStructTy I assume you may move this under "if (ClInitializers)"? Same is true for filling DynamicInit with contents. We can also cut off calling FindDynamicInitializers/HasDynamicInitializer methods, but then __asan_globals will always have "has_dynamic_initializer" equal to false, which may be confusing... Anyway, I suggest to hide everything under the flag for commit, and measure performance first.
Sign in to reply to this message.
Looks good, please commit. http://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentati... File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right): http://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentati... lib/Transforms/Instrumentation/AddressSanitizer.cpp:383: assert(G); use cast<> and remove assert
Sign in to reply to this message.
Several style nits. https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentat... File lib/Transforms/Instrumentation/AddressSanitizer.cpp (right): https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentat... lib/Transforms/Instrumentation/AddressSanitizer.cpp:370: M.getNamedMetadata("dynamically_initialized_globals"); I think it's more common to use 4-space indentation when breaking a line this way. https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentat... lib/Transforms/Instrumentation/AddressSanitizer.cpp:387: // Returns true if a global variable is initialized dynamically in this TU. Please insert a newline before the function declaration. Consider also running `make -f Makefile.old lint` in the ASan runtime directory. https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentat... lib/Transforms/Instrumentation/AddressSanitizer.cpp:508: Please remove the spare newlines https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentat... lib/Transforms/Instrumentation/AddressSanitizer.cpp:600: Spare newlines. https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentat... lib/Transforms/Instrumentation/AddressSanitizer.cpp:609: E = M.global_end(); G != E; ++G) { "E =" should be right below "Module" https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentat... lib/Transforms/Instrumentation/AddressSanitizer.cpp:636: Value *FirstDynamic = 0, *LastDynamic = 0; It's better to use NULL for pointers. https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentat... lib/Transforms/Instrumentation/AddressSanitizer.cpp:688: if ( FirstDynamic == 0 ) Inconsistent spacing around the expression. Also, why not just (!FirstDynamic) ?
Sign in to reply to this message.
On Thu, Aug 16, 2012 at 12:57 PM, <glider@chromium.org> wrote: > Several style nits. > > > https://codereview.appspot.**com/6432065/diff/13004/lib/** > Transforms/Instrumentation/**AddressSanitizer.cpp<https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentation/AddressSanitizer.cpp> > File lib/Transforms/**Instrumentation/**AddressSanitizer.cpp (right): > > https://codereview.appspot.**com/6432065/diff/13004/lib/** > Transforms/Instrumentation/**AddressSanitizer.cpp#**newcode370<https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode370> > lib/Transforms/**Instrumentation/**AddressSanitizer.cpp:370: > M.getNamedMetadata("**dynamically_initialized_**globals"); > I think it's more common to use 4-space indentation when breaking a line > this way. > > https://codereview.appspot.**com/6432065/diff/13004/lib/** > Transforms/Instrumentation/**AddressSanitizer.cpp#**newcode387<https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode387> > lib/Transforms/**Instrumentation/**AddressSanitizer.cpp:387: // Returns > true > if a global variable is initialized dynamically in this TU. > Please insert a newline before the function declaration. Consider also > running `make -f Makefile.old lint` in the ASan runtime directory. > > https://codereview.appspot.**com/6432065/diff/13004/lib/** > Transforms/Instrumentation/**AddressSanitizer.cpp#**newcode508<https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode508> > lib/Transforms/**Instrumentation/**AddressSanitizer.cpp:508: > Please remove the spare newlines > > https://codereview.appspot.**com/6432065/diff/13004/lib/** > Transforms/Instrumentation/**AddressSanitizer.cpp#**newcode600<https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode600> > lib/Transforms/**Instrumentation/**AddressSanitizer.cpp:600: > Spare newlines. > > https://codereview.appspot.**com/6432065/diff/13004/lib/** > Transforms/Instrumentation/**AddressSanitizer.cpp#**newcode609<https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode609> > lib/Transforms/**Instrumentation/**AddressSanitizer.cpp:609: E = > M.global_end(); G != E; ++G) { > "E =" should be right below "Module" > > https://codereview.appspot.**com/6432065/diff/13004/lib/** > Transforms/Instrumentation/**AddressSanitizer.cpp#**newcode636<https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode636> > lib/Transforms/**Instrumentation/**AddressSanitizer.cpp:636: Value > *FirstDynamic = 0, *LastDynamic = 0; > It's better to use NULL for pointers. > I remember Chandler asking to use 0 instead of NULL. For me 0 is fine. > > https://codereview.appspot.**com/6432065/diff/13004/lib/** > Transforms/Instrumentation/**AddressSanitizer.cpp#**newcode688<https://codereview.appspot.com/6432065/diff/13004/lib/Transforms/Instrumentation/AddressSanitizer.cpp#newcode688> > lib/Transforms/**Instrumentation/**AddressSanitizer.cpp:688: if ( > FirstDynamic == 0 ) > Inconsistent spacing around the expression. > Also, why not just (!FirstDynamic) ? > > https://codereview.appspot.**com/6432065/<https://codereview.appspot.com/6432... >
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
