[llvm-commits] LLVM changes for detecting initialization order problems in Address Sanitizer

[Kostya Serebryany]

On Mon, Jul 2, 2012 at 11:34 AM, Kostya Serebryany <kcc at google.com> wrote:

> Hi Reid,
>
> +// This flag may need to be replaced with -fasan-initializers.
> Ideally, this phase should be on-by default when asan is enabled.
> Assuming it will be robust and have no false reports, of course.
> For now, -mllvm -asan-initializers is ok for me.
>
>
>    size_t n = GlobalsToChange.size();
>
>
> +  NumGlobals = n;
>
> Please remove 'n'.
>
> +    if (ClInitializers && F->getName() == "_GLOBAL__I_a")
>
>
>
> This is wrong. We should not identify global init functions using the
> name.
> There should be some other way (attribute? metadata?)
>


>
> +  // Add a call to poison all external globals before the given function
> starts
>
>
> Ouch. This will make the algorithm quadratic by the number of global
> variables.
> Essentially, we poison all globals each time we initialize a global.
> Instead, we need to poison all globals of all modules (except for the
> current) when we start handling a given module.
> This will make it quadratic by the number of modules, which is tolerable.
>

I am wrong here, you seem to be inserting the callbacks into the right
place.
Than another comment:

+  SmallVector<ReturnInst*, 8> ReturnInstructions;


Do you need this temp? Maybe have one loop instead?



>
> Besides, I don't see how you distinguish between globals that have
> run-time init and linker-initialized-globals.
> In particular, the following is sad:
> +  if (isa<GlobalVariable>(Addr) && ClOpt && ClOptGlobals &&
> !ClInitializers) {
>
> Here you disable a performance optimization that (last time I measured)
> gave ~1% of speedup.
> Meanwhile, we still don't need to touch linker-initialized globals.
>
> May I ask you to send next asan patches using rietveld (in addition to
> attaching the patch to the message)?
> I know that LLVM community does not require that, but my code review
>  productivity drops when I look at raw patches.
>
> Thanks!
>
>
>
>
>
> On Sat, Jun 30, 2012 at 1:36 AM, Reid Watson <reidw at google.com> wrote:
>
>> Hello,
>>
>> I've attached a patch to add basic support for detecting problems with
>> initialization order in C++ to AddressSanitizer.
>> This patch relies on changes to the runtime library, and I've sent a
>> patch for this to cfe-commits.
>> This is definitely a first draft, and it leans toward false positives
>> (function local statics, in particular), but it does detect the most
>> basic cases of the "static initialization order fiasco".
>> For now, I'd like to get a working baseline/infrastructure committed,
>> in order to avoid any monster commits.
>>
>> All the best,
>> Reid
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120702/b16a7ebb/attachment.html>