On Tue, Jun 26, 2012 at 2:09 PM, Eli Friedman <[email protected]> wrote: > On Tue, Jun 26, 2012 at 1:54 PM, David Blaikie <[email protected]> wrote: >> On Fri, Jun 22, 2012 at 4:50 PM, Chandler Carruth <[email protected]> >> 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
crash_rewrite.diff
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
