[cfe-commits] [llvm-commits] [PATCH] AddressSanitizer Initialization Order Checking

[Kostya Serebryany]

On Tue, Aug 21, 2012 at 11:36 AM, Kostya Serebryany <kcc at google.com> wrote:

> submitted the clang part: r162259.
> Looking at the rest (there are minor problems, I may be able to fix them
> myself).
>

LLVM part went as r162268 with minor changes:

1. Don't call createInitializerPoisonCalls if there are no globals with
dynamic init in the current TU.
2. Fix test instrument_initializer_metadata.ll to have correct metadata
name
3. removed instrument_initializer.ll -- I did not understand why this test
was needed. Please comment.

Looking at the compiler-rt part, where I don't like some parts.

--kcc


>
>
> --kcc
>
>
> On Tue, Aug 21, 2012 at 12:52 AM, Reid Watson <reidw at google.com> wrote:
>
>> OK, I re-reviewed what was contained in the patches.
>> I had a few non-reviewed experiments accidentally included in the
>> previous patches.
>> I've attached the correct versions, containing only the properly
>> reviewed changes.
>> Sincere apologies for the trouble!
>>
>> On Mon, Aug 20, 2012 at 1:14 PM, Reid Watson <reidw at google.com> wrote:
>> > Sorry, minor problem with the compiler-rt patch.
>> >
>> > Please commit the attached version of the compiler-rt patch instead of
>> > the previous one.
>> >
>> > On Mon, Aug 20, 2012 at 10:57 AM, Reid Watson <reidw at google.com> wrote:
>> >> I've incorporated all the review comments in the attached patches.
>> >> The tool is off by default, and enabled by adding -mllvm
>> >> -asan-initialization-order to clang.
>> >>
>> >> I don't have commit access, so I'd appreciate it if someone could
>> >> commit these for me.
>> >>
>> >> All the best,
>> >> Reid
>> >>
>> >> On Thu, Aug 16, 2012 at 6:09 PM, Reid Watson <reidw at google.com> wrote:
>> >>> I'll add in the prefix before the final version for commit.
>> >>>
>> >>> The goal is to detect globals with dynamic initialization per the C++
>> >>> standard, so that ASan knows which globals can only be accessed during
>> >>> .  It's valid to access a statically initialized global inside of an
>> >>> initializer, even if that global resides in a different TU.  Thus,
>> >>> LLVM/ASan need to be able to distinguish between
>> >>> dynamically/statically initialized globals.  I'm not certain this is
>> >>> optimal, but there haven't been any unexpected false positives with
>> >>> it.  We may be missing a few case, and the optimizer can certainly end
>> >>> up leaving us with false negatives, but those are much better than
>> >>> false positives.
>> >>>
>> >>> On Thu, Aug 16, 2012 at 5:27 PM, Eric Christopher <echristo at apple.com>
>> wrote:
>> >>>>
>> >>>>
>> >>>> On Aug 16, 2012, at 1:05 AM, Kostya Serebryany <kcc at google.com>
>> wrote:
>> >>>>
>> >>>>> +llvm-commits
>> >>>>>
>> >>>>> Reid,
>> >>>>>
>> >>>>> The LLVM and compiler-rt patches look good.
>> >>>>> Please fix the remaining small issues (see my code review comments)
>> and commit.
>> >>>>> Hold on with the output tests for a bit since Alexey Samsonov is
>> migrating them to cmake (please coordinate with him and commit as a
>> separate patch).
>> >>>>>
>> >>>>> The stress test should contain X files, Y linker initialized
>> globals and Z dynamically initialized globals.
>> >>>>> Such test only makes sense where all 3 numbers are large.
>> >>>>> I guess you can commit a single .sh script into
>> compiler-rt/lib/asan/scripts
>> >>>>
>> >>>> The metadata should at least be prefixed with something like
>> llvm.asan.<whatever> instead of just the name. That way it's more
>> identifiable.
>> >>>>
>> >>>> What's the idea behind the metadata use anyhow?
>> >>>>
>> >>>> -eric
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120821/d5c5f06f/attachment.html>