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

[David Blaikie]

*vigorous ping*

On Wed, Jun 27, 2012 at 10:10 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 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/20120629/33ebe14e/attachment.obj>