[PATCH] Add CHECK-LABEL directive to FileCheck to allow more accurate error messages and error recovery

Michael Gottesman mgottesman at apple.com
Thu Jul 11 13:37:53 PDT 2013 

Perfect = ).

Michael

On Jul 11, 2013, at 1:35 PM, Daniel Dunbar <daniel at zuster.org> wrote:

> On Thu, Jul 11, 2013 at 12:58 PM, Michael Gottesman <mgottesman at apple.com> wrote:
> I proposed the same thing some time ago and IIRC Daniel had some problem with it (which I don’t remember right now).
> 
> No, I don't have any problem with this.
> 
>  - Daniel
>  
> 
> (CCing Daniel if he has any input).
> 
> Michael
> 
> On Jul 11, 2013, at 12:54 PM, Eli Bendersky <eliben at google.com> wrote:
> 
>> 
>> On Thu, Jul 11, 2013 at 12:44 PM, Stephen Lin <swlin at post.harvard.edu> wrote:
>> Actually, I would be ok with CHECK-BOUND as well.
>> Eli, is that OK to you? And does anyone else want to chime in?
>> I will expand the docs either way.
>> Thanks,
>> Stephen
>> 
>> I'm not sure what BOUND means in this case? And how is it different from BOUNDARY? 
>> 
>> I'm just thinking of someone reading the test file and looking at all the directives. BOUNDARY conveys a spatial meaning and it's easy to intuitively remember what its semantics are. My opposition to LABEL was because LABEL conveyed no such meaning and I think it would be confusing. As for BOUND vs. BOUNDARY, that's really a minor issue and perhaps my knowledge of English fails me here, but I'd be happy to hear the reasoning.
>> 
>> Eli
>> 
>> 
>> 
>> 
>>  
>> 
>> On Thu, Jul 11, 2013 at 12:40 PM, Stephen Lin <swlin at post.harvard.edu> wrote:
>> > Thanks Owen; Andy (Trick) off-list says he thinks it's a good idea, too.
>> >
>> > Eli B. (also off-list) thinks that the documentation can be approved
>> > and also suggests that the name CHECK-BOUNDARY is better. Anyone else
>> > have an opinion?
>> >
>> > I much prefer CHECK-LABEL to CHECK-BOUNDARY myself, but I am willing
>> > to paint the bike shed whatever color others can agree on.
>> >
>> > Stephen
>> >
>> > On Thu, Jul 11, 2013 at 12:31 PM, Owen Anderson <resistor at mac.com> wrote:
>> >> I'm not familiar enough with the FileCheck internals to comment on the implementation, but I *really* like this feature.  I've spent way too much time over the years tracking down cryptic FileCheck errors that would have been solved by this.
>> >>
>> >> --Owen
>> >>
>> >> On Jul 11, 2013, at 10:50 AM, Stephen Lin <swlin at post.harvard.edu> wrote:
>> >>
>> >>> Hi,
>> >>>
>> >>> Can anyone review this patch? It adds a new directive type called
>> >>> "CHECK-LABEL" to FileCheck...
>> >>>
>> >>> If present in a match file, FileCheck will use these directives to
>> >>> split the input into blocks that are independently processed, ensuring
>> >>> that a CHECK does not inadvertently match a line in a different block
>> >>> (which can lead to a misleading/useless error message when the error
>> >>> is eventually caught). Also, FileCheck can now recover from errors
>> >>> within blocks by continuing to the next block.
>> >>>
>> >>> As an example, I purposely introduced the a switch fall-through bug in
>> >>> the last patch I submitted to llvm-commits ("Allow FMAs in safe math
>> >>> mode in some cases when one operand of the fmul is either exactly 0.0
>> >>> or exactly 1.0")...
>> >>>
>> >>> Bug diff:
>> >>>
>> >>> diff --git a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> >>> b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> >>> index 0290afc..239b119 100644
>> >>> --- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> >>> +++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> >>> @@ -5791,7 +5791,7 @@ static bool isExactlyZeroOrOne(const
>> >>> TargetLowering &TLI, const SDValue &Op) {
>> >>>           continue;
>> >>>         }
>> >>>       }
>> >>> -      break;
>> >>> +//      break;
>> >>>     case ISD::FADD:
>> >>>       if (ConstantFPSDNode *V0CFP =
>> >>>             dyn_cast<ConstantFPSDNode>(V->getOperand(0))) {
>> >>>
>> >>> The single error message without CHECK-LABEL is:
>> >>>
>> >>> ; CHECK-SAFE: test_add_8
>> >>>              ^
>> >>> <stdin>:125:2: note: scanning from here
>> >>> .cfi_endproc
>> >>> ^
>> >>> <stdin>:127:10: note: possible intended match here
>> >>> .globl _test_add_10
>> >>>         ^
>> >>>
>> >>> The error messages with CHECK-LABEL are:
>> >>>
>> >>> ; CHECK-SAFE: vmulsd
>> >>>              ^
>> >>> <stdin>:87:2: note: scanning from here
>> >>> .align 4, 0x90
>> >>> ^
>> >>> <stdin>:95:2: note: possible intended match here
>> >>> vsubsd %xmm0, %xmm3, %xmm0
>> >>> ^
>> >>> fp-contract.ll:118:15: error: expected string not found in input
>> >>> ; CHECK-SAFE: vmulsd
>> >>>              ^
>> >>> <stdin>:102:2: note: scanning from here
>> >>> .align 4, 0x90
>> >>> ^
>> >>> <stdin>:109:2: note: possible intended match here
>> >>> vsubsd %xmm2, %xmm3, %xmm2
>> >>> ^
>> >>> fp-contract.ll:288:15: error: expected string not found in input
>> >>> ; CHECK-SAFE: vmulsd
>> >>>              ^
>> >>> <stdin>:258:2: note: scanning from here
>> >>> .align 4, 0x90
>> >>> ^
>> >>> <stdin>:266:2: note: possible intended match here
>> >>> vsubsd %xmm0, %xmm3, %xmm0
>> >>> ^
>> >>>
>> >>> The three error messages in the CHECK-LABEL case exactly pinpoint the
>> >>> source lines of the actual problem in three separate blocks; the
>> >>> single error message given without CHECK-LABEL is (imho) much less
>> >>> useful.
>> >>>
>> >>> (In this case, the non-CHECK-LABEL version happens to error on the on
>> >>> a label line, so the user could presume that the error happened in the
>> >>> block immediately before test_add_8, which is correct, but in general
>> >>> this might not be true; the only thing that can be concluded is that
>> >>> the error happened sometime before test_add_8.)
>> >>>
>> >>> Please let me know if you have any feedback.
>> >>>
>> >>> Stephen
>> >>>
>> >>> ---------- Forwarded message ----------
>> >>> From: Stephen Lin <swlin at apple.com>
>> >>> Date: Mon, Jun 10, 2013 at 4:21 PM
>> >>> Subject: [PATCH] Add CHECK-LABEL directive to FileCheck to allow more
>> >>> accurate error messages and error recovery
>> >>> To: llvm-commits at cs.uiuc.edu
>> >>>
>> >>>
>> >>> Actually, I went ahead and renamed it CHECK-LABEL and rebased, since I
>> >>> think it’s better :)
>> >>> -Stephen
>> >>> <check-label.patch>_______________________________________________
>> >>> llvm-commits mailing list
>> >>> llvm-commits at cs.uiuc.edu
>> >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >>
>> >>
>> >> _______________________________________________
>> >> llvm-commits mailing list
>> >> llvm-commits at cs.uiuc.edu
>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130711/81f18064/attachment.html>

More information about the llvm-commits mailing list