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

[David Blaikie]

On Fri, Jun 29, 2012 at 2:55 PM, Chandler Carruth <chandlerc at google.com> wrote:
> Thanks for poking!
>
> diff --git lib/Lex/Pragma.cpp lib/Lex/Pragma.cpp
> index 7b94d26..c9cc4ad 100644
> --- lib/Lex/Pragma.cpp
> +++ lib/Lex/Pragma.cpp
> @@ -1010,6 +1010,10 @@ struct PragmaDebugHandler : public PragmaHandler {
>        llvm_unreachable("This is an assertion!");
>      } else if (II->isStr("crash")) {
>        *(volatile int*) 0x11 = 0;
>
> Wait, what? __builtin_trap maybe?
>
> +    } else if (II->isStr("parser_crash")) {
> +      Token Crasher;
> +      Crasher.setKind(tok::annot_pragma_parser_crash);
> +      PP.EnterToken(Crasher);
>      } else if (II->isStr("llvm_fatal_error")) {
>        llvm::report_fatal_error("#pragma clang __debug llvm_fatal_error");
>      } else if (II->isStr("llvm_unreachable")) {
> diff --git lib/Parse/ParseDecl.cpp lib/Parse/ParseDecl.cpp
> index 489a46c..409f8e0 100644
> --- lib/Parse/ParseDecl.cpp
> +++ lib/Parse/ParseDecl.cpp
> @@ -4252,6 +4252,8 @@ void Parser::ParseDirectDeclarator(Declarator &D) {
>      // portion is empty), if an abstract-declarator is allowed.
>      D.SetIdentifier(0, Tok.getLocation());
>    } else {
> +    if (Tok.getKind() == tok::annot_pragma_parser_crash)
> +      *(volatile int*) 0x11 = 0;
>
> Yea, here too.
>
> Don't fix this in this patch -- I expect we'll need to try a few times to
> get a portable incantation of __builtin_trap, likely involving a Compiler.h
> bit of magic to do the volatile pointer thwack when the compiler doesn't
> have __builtin_trap.

I see - I'll look into what that'll take.

> --- /dev/null
> +++ test/Driver/crash-report.c
> @@ -0,0 +1,6 @@
> +// RUN: %clang -fsyntax-only %s 2>&1 | FileCheck %s
> +// REQUIRES: crash-recovery
> +
> +#pragma clang __debug parser_crash
> +// CHECK: Preprocessed source(s) and associated run script(s) are located
> at:
> +// CHECK-NEXT: clang-3: note: diagnostic msg: {{.*}}.c
>
> Isn't this going to litter 'tmp' with crash reductions?

Yes, I suppose it will, unfortunately.

> I think our bots
> will be sad. Is there a tool to redirect the output to a specific file or
> directory so we can put it in the expected %t-related place fro the
> testsuite?

I don't believe there is. This seems to go straight into
Driver::GetTemporaryPath which looks for a bunhc of env variables
(TMPDIR, TEMP, TMP) then falls back to a hardcoded path (/tmp) and
makes a file there.

I could add a flag to control the behavior of this function in
general, or specifically for crash reproductions.

The other piece though: is it possible to retrieve the directory of
lit's %t so I can pass that to Clang once it has such a flag?

Committed without further changes as r159469 & I'll address the
trapping & temp file locations as follow up based on
discussion/investigation.

- David

>
>
> On Fri, Jun 29, 2012 at 2:42 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> *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
>
>