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

Mon Jun 30 03:06:47 PDT 2014

Yep, that one fixes the issue as well and passes regression tests.
Disregard my comment about Entry.get. Are you ok with me committing his
version Richard? With ampersand aligned with variable of course :P

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

> 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/2afc8f9d/attachment.html>