[cfe-commits] [patch] rewrite-includes crash support

[David Blaikie]

On Tue, Jun 26, 2012 at 2:09 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> On Tue, Jun 26, 2012 at 1:54 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> On Fri, Jun 22, 2012 at 4:50 PM, Chandler Carruth <chandlerc at google.com> wrote:
>>> Doh, sorry for missing this last patch.
>>>
>>> Looks pretty good. As discussed in person, you may want to sink the arg
>>> mutation below the PrintJob... Not sure.
>>
>> We also discussed the possibility of avoiding mutating the original
>> args - did you/we end up dismissing that since we're mutating the jobs
>> anyway, so there's no real immutability invariant that we'd be
>> mantaining?
>>
>> As for moving it, if it's all the same to you, I think I'll just leave
>> it where it is next to the point at which we raise the CCCIsCPP flag
>> as they seem related. (I could move all of that down below PrintJob I
>> suppose, though... )
>>
>>> I'm concerned by the lack of test case modifications. If we don't have a
>>> test case yet for this, lets get one and check the filename. If we do, we
>>> should tighten it up to check the filename or figure out whats going on.
>>
>> Agreed, but how exactly would I do that? The existing mechanism we
>> have for crashing clang is "#pragma clang __debug crash" which crashes
>> in the preprocessor. That means it crashes when crash recovery
>> attempts to produce the preprocessed source file. As we discussed
>> offline there's probably a couple of options here:
>>
>> 1) Transform the pragma into a special token then look for that token
>> in the parser & crash there. But how do we do this without adding a
>> test to a very hot part of the parser?
>
> We don't necessarily need to crash anywhere we see the token in
> question; we could say it's only "legal" in places where a top-level
> declaration would be allowed.

Ah, that certainly helps.

So I've implemented a basic version of this - all I did was find where
we handle the crash pragma, added another "parser_crash" and inserted
a token (annot_pragma_parser_crash) into the stream. Then I found
where that crashed in the parser & added an explicit check/crash for
it. This still means you'll get arbitrary behavior/crashes if you use
this pragma elsewhere - is that acceptable "invalid" behavior for such
an implementation detail, or should I make this more robust in some
way?

Attached is the parser_crash support, test case (testing only the file
extension mentioned in the crash report - is there some way I could
actually open that file from within lit so I could FileCheck it? I
guess that's just not worth the effort?) and the original
functionality I'd sent for review.

- David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: crash_rewrite.diff
Type: application/octet-stream
Size: 5531 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120627/39234497/attachment.obj>