*vigorous ping*
On Wed, Jun 27, 2012 at 10:10 AM, David Blaikie <[email protected]> wrote: > 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
