[cfe-dev] Clang produces corrupt pch leading to crash

Mon Jun 30 01:39:48 PDT 2014

Ignore my last email.

Richard's change (plus const_cast and changing Entry. to Entry->) fixes the
issue. The patch Tobias proposed should also work and might be a little
cleaner? It definitely needs a comment justifying the second lookup.
There's also no need to call Entry.get in this case as we know the pointer
is null?

On Mon, Jun 30, 2014 at 6:22 PM, Nikola Smiljanic <popizdeh at gmail.com>
wrote:

> I was about to do the same :) But one thing I wanted to ask is, why change
> Entry into a pointer? Why not just call operator[] twice? As Tobias said
> we'll need to cast to CXXRecordDecl. And we'll have to change Entry.get to
> Entry->get if Entry needs to be a pointer.
>
>
> On Mon, Jun 30, 2014 at 5:42 PM, Tobias Hahn <tobias.hahn at ableton.com>
> wrote:
>
>>  Hey Richard,
>>
>> I'll check your patch in a minute, but except for a missing const_cast I
>> believe it should be fine just by comparing it with the patch I came up
>> with (attached below).
>>
>> Best,
>> Tobias
>>   Am 27.06.2014 um 18:24 schrieb Richard Smith <richard at metafoo.co.uk>:
>>
>>
>> > I think this may be one of those changes for which we need to accept
>> that a unit test would not be useful. Does the change I suggested upthread
>> fix the issue for you?
>> >
>> > On Fri, Jun 27, 2014 at 9:15 AM, Tobias Hahn <tobias.hahn at ableton.com>
>> wrote:
>> > The last test case I sent around is as minimal as I could get it, but
>> still over a hundred loc. It only crashes reliably when clang is built with
>> asan (valgrind would probably be another option). I've tried a patch very
>> similar to yours; and it fixes the issue.
>> >
>> > Maybe one could construct a simpler test case with the knowledge that
>> it has to force the KeyFunctions DenseMap to re-allocate?
>> >
>> > I've also thought about how to unit-test this, but it seems impossible
>> to write a unit test without exposing the KeyFunctions cache in ASTContext.
>> At this point I stopped because I have no idea what your policy is for such
>> changes and in what general directions this code is meant to evolve.
>> >
>> > Best,
>> > Tobias
>> >
>> > Am 27.06.2014 um 17:24 schrieb "Richard Smith" <richard at metafoo.co.uk>:
>> >
>> >> On Sun, Jun 22, 2014 at 5:45 PM, Nikola Smiljanic <popizdeh at gmail.com>
>> wrote:
>> >> Awesome work Tobias, very interesting sequence of events. Adding
>> Richard as he'll know how to best reshuffle this code :D
>> >>
>> >> The precompiled header is good, the trouble starts in
>> ASTContext::getCurrentKeyFunctions:
>> >>
>> >> LazyDeclPtr &Entry = KeyFunctions[RD];
>> >>   if (!Entry)
>> >>     Entry = const_cast<CXXMethodDecl*>(computeKeyFunction(*this, RD));
>> >>
>> >> That's a classic :)
>> >>
>> >> It should be straightforward to fix this, something like:
>> >>
>> >> LazyDeclPtr *Entry = &KeyFunctions[RD];
>> >> if (!*Entry) {
>> >>   auto *KeyFunction = computeKeyFunction(*this, RD);
>> >>   // Entry might have been invalidated by computeKeyFunction.
>> >>   Entry = &KeyFunctions[RD];
>> >>   *Entry = KeyFunction;
>> >> }
>> >> // do stuff with *Entry
>> >>
>> >> Do we have a reduced testcase? I suspect that such a test may be
>> fragile to the point of being useless, so maybe we should go without =(
>> >>
>> >> KeyFunctions DenseMap will undergo reallocation inside
>> computeKeyFunctions (see two markers in the attached callstack). Upon
>> return Entry will point to freed memory. Good lord it took me a while to
>> figure this out, I suck at debugging on Linux...
>> >>
>> >>
>> >> On Sat, Jun 21, 2014 at 6:12 AM, Tobias Hahn <tobias.hahn at ableton.com>
>> wrote:
>> >> Hi Nikola,
>> >>
>> >> I can reproduce the bug on linux if I tell clang to cross-compile for
>> osx. Attached is a minimal example. You need to build clang with asan to
>> reproduce the crash:
>> >>
>> >> ../llvm/configure --enable-libcpp CFLAGS="-fsanitize=address"
>> CXXFLAGS="-fsanitize=address"
>> >>
>> >> Thanks again for looking into this crash!
>> >>
>> >> Best,
>> >> Tobias
>> >>
>> >> Am 17.06.2014 um 00:16 schrieb Nikola Smiljanic <popizdeh at gmail.com>:
>> >>
>> >>
>> >> > It might make sense to file this with apple if you're using clang
>> shipped with XCode as they ship their own releases. What version of clang
>> are you using? Is the isystem flag important? What about #include <list>?
>> I've tried to reproduce this but it's not so straightforward because I'm on
>> linux and some of the stuff in that bash script assumes mac os... I was
>> wondering if it's possible to reduce this to something that's reproducible
>> everywhere or if this is a mac specific issue.
>> >> >
>> >> >
>> >> > On Mon, Jun 16, 2014 at 11:55 PM, Nikola Smiljanic <
>> popizdeh at gmail.com> wrote:
>> >> > Thanks for the detailed report! The only thing you can do more is
>> try and debug this yourself ;)
>> >> >
>> >> >
>> >> > On Mon, Jun 16, 2014 at 8:33 PM, Tobias Hahn <
>> tobias.hahn at ableton.com> wrote:
>> >> > Hi all,
>> >> >
>> >> > I've run into (what I believe is) a memory bug with clang while
>> producing a precompiled header. In short, under certain circumstances,
>> clang will write a pch that causes a crash when trying to use this pch in a
>> later compilation unit.
>> >> >
>> >> > Occasionally, while clang is compiling the pch, malloc complains
>> that one of its checksums has been overwritten; while at other times, clang
>> throws an error that a definition has a different exception specification
>> than the declaration two lines above it (when both have no exception
>> specification). Both these symptoms lead me to believe that somewhere clang
>> overwrites memory.
>> >> >
>> >> > I have stripped the code that reliably causes this crash down to a
>> few hundred lines and have created a little script to reproduce the bug
>> (details at http://llvm.org/bugs/show_bug.cgi?id=20026). I'm not sure,
>> however, about your process for handling such bugs, which is why I'm
>> cross-posting here. My main question is if there is anything else I could
>> provide you with to help fixing this issue.
>> >> >
>> >> > Thank you very much in advance!
>> >> >
>> >> > Best,
>> >> > Tobias
>> >> >
>> >> >
>> >> > Ableton AG, Schoenhauser Allee 6-7, 10119 Berlin, Germany
>> >> > Sitz (Registered Office) Berlin, Amtsgericht Berlin-Charlottenburg,
>> HRB 72838
>> >> > Vorstand (Management Board): Gerhard Behles, Jan Bohl
>> >> > Vorsitzender des Aufsichtsrats (Chair of the Supervisory Board): Uwe
>> Struck
>> >> >
>> >> >
>> >> >
>> >> > _______________________________________________
>> >> > cfe-dev mailing list
>> >> > cfe-dev at cs.uiuc.edu
>> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>> >> >
>> >> >
>> >>
>> >> Ableton AG, Schoenhauser Allee 6-7, 10119 Berlin, Germany
>> >> Sitz (Registered Office) Berlin, Amtsgericht Berlin-Charlottenburg,
>> HRB 72838
>> >> Vorstand (Management Board): Gerhard Behles, Jan Bohl
>> >> Vorsitzender des Aufsichtsrats (Chair of the Supervisory Board): Uwe
>> Struck
>> >> >>
>> >>
>> >>
>> >>
>> >
>>
>> Ableton AG, Schoenhauser Allee 6-7, 10119 Berlin, Germany
>> Sitz (Registered Office) Berlin, Amtsgericht Berlin-Charlottenburg, HRB
>> 72838
>> Vorstand (Management Board): Gerhard Behles, Jan Bohl
>> Vorsitzender des Aufsichtsrats (Chair of the Supervisory Board): Uwe
>> Struck
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140630/4f8117cc/attachment.html>