[llvm] r226627 - Make DIExpression::Verify() stricter by checking that the number of
Adrian Prantl
aprantl at apple.com
Wed Jan 21 08:01:53 PST 2015
> On Jan 20, 2015, at 6:05 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
>
>> On 2015 Jan 20, at 16:59, Adrian Prantl <aprantl at apple.com> wrote:
>>
>> Author: adrian
>> Date: Tue Jan 20 18:59:20 2015
>> New Revision: 226627
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=226627&view=rev
>> Log:
>> Make DIExpression::Verify() stricter by checking that the number of
>> elements and the ordering is sane and cleanup the accessors.
>>
>
> Funny, I was just wondering earlier today what sort of expressions are
> valid.
>
>> Added:
>> llvm/trunk/test/DebugInfo/piece-verifier.ll
>> Modified:
>> llvm/trunk/include/llvm/IR/DebugInfo.h
>> llvm/trunk/lib/IR/DebugInfo.cpp
>> llvm/trunk/lib/IR/Verifier.cpp
>>
>> Modified: llvm/trunk/include/llvm/IR/DebugInfo.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DebugInfo.h?rev=226627&r1=226626&r2=226627&view=diff
>> ==============================================================================
>> --- llvm/trunk/include/llvm/IR/DebugInfo.h (original)
>> +++ llvm/trunk/include/llvm/IR/DebugInfo.h Tue Jan 20 18:59:20 2015
>> @@ -1036,6 +1036,9 @@ public:
>> /// \brief Process DILocation.
>> void processLocation(const Module &M, DILocation Loc);
>>
>> + /// \brief Process DIExpression.
>> + void processExpression(DIExpression Expr);
>> +
>> /// \brief Clear all lists.
>> void reset();
>>
>>
>> Modified: llvm/trunk/lib/IR/DebugInfo.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=226627&r1=226626&r2=226627&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/IR/DebugInfo.cpp (original)
>> +++ llvm/trunk/lib/IR/DebugInfo.cpp Tue Jan 20 18:59:20 2015
>> @@ -148,17 +148,18 @@ uint64_t DIExpression::getElement(unsign
>> }
>>
>> bool DIExpression::isVariablePiece() const {
>> - return getNumElements() && getElement(0) == dwarf::DW_OP_piece;
>> + unsigned N = getNumElements();
>> + return N >=3 && getElement(N-3) == dwarf::DW_OP_piece;
>> }
>>
>> uint64_t DIExpression::getPieceOffset() const {
>> - assert(isVariablePiece());
>> - return getElement(1);
>> + assert(isVariablePiece() && "not a piece");
>> + return getElement(getNumElements()-2);
>> }
>>
>> uint64_t DIExpression::getPieceSize() const {
>> - assert(isVariablePiece());
>> - return getElement(2);
>> + assert(isVariablePiece() && "not a piece");
>> + return getElement(getNumElements()-1);
>> }
>>
>> //===----------------------------------------------------------------------===//
>> @@ -593,6 +594,24 @@ bool DIExpression::Verify() const {
>> if (!DbgNode)
>> return true;
>>
>> + unsigned N = getNumElements();
>> + for (unsigned I = 0; I < N; ++I)
>> + switch (getElement(I)) {
>> + case DW_OP_piece:
>> + // DW_OP_piece has to be the last element in the expression and take two
>> + // arguments.
>> + if (getElement(I) == DW_OP_piece && !isVariablePiece())
>
> The first part of the condition looks always true. Did you mean something
> else (or am I confused?)?
Right. I turned an if-cascade into a switch statement and this was left over.
> Also, I'm not sure how/whether you've actually checked that this is the
> last element.
isVariablePiece() looks at the last element of the expression and sees whether it’s a DW_OP_piece. Which is, now that I think about it, not good enough. It really needs to skip forward through the expression so it doesn’t accidentally treat an integer argument as a DW_OP.
I’ll make another pass on this.
thanks,
adrian
>
>> + return false;
>> + I += 2;
>> + break;
>> + case DW_OP_plus:
>> + // Takes one argument.
>> + if (I+1 == N)
>
> (Nit: I think we prefer `I + 1`.)
>
>> + return false;
>> + I += 1;
>
> You might just simplify this all to:
>
> if (++I == N)
> return false;
>
>> + break;
>> + default: break;
>
> Does this mean "don't know how to verify", or should this just be
> rejected?
>
> In the former case, I wonder if you should break the `for` loop entirely,
> since there could be some argument to an unrecognized operator that
> happens to be equal to a recognized `DW_OP_`. In the latter case,
> shouldn't you just return `false`?
>
>> + }
>
> I feel like all of this new code should go *after* the more basic
> verification.
>
>> return isExpression() && DbgNode->getNumOperands() == 1;
>
> I.e., here.
>
>> }
>>
>>
>> Modified: llvm/trunk/lib/IR/Verifier.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Verifier.cpp?rev=226627&r1=226626&r2=226627&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/IR/Verifier.cpp (original)
>> +++ llvm/trunk/lib/IR/Verifier.cpp Tue Jan 20 18:59:20 2015
>> @@ -2841,12 +2841,20 @@ void DebugInfoVerifier::processCallInst(
>> if (Function *F = CI.getCalledFunction())
>> if (Intrinsic::ID ID = (Intrinsic::ID)F->getIntrinsicID())
>> switch (ID) {
>> - case Intrinsic::dbg_declare:
>> - Finder.processDeclare(*M, cast<DbgDeclareInst>(&CI));
>> + case Intrinsic::dbg_declare: {
>> + auto *DDI = cast<DbgDeclareInst>(&CI);
>> + Finder.processDeclare(*M, DDI);
>> + if (auto E = DDI->getExpression())
>> + Assert1(DIExpression(E).Verify(), "DIExpression does not Verify!", E);
>> break;
>> - case Intrinsic::dbg_value:
>> - Finder.processValue(*M, cast<DbgValueInst>(&CI));
>> + }
>> + case Intrinsic::dbg_value: {
>> + auto *DVI = cast<DbgValueInst>(&CI);
>> + Finder.processValue(*M, DVI);
>> + if (auto E = DVI->getExpression())
>> + Assert1(DIExpression(E).Verify(), "DIExpression does not Verify!", E);
>> break;
>> + }
>> default:
>> break;
>> }
>>
>> Added: llvm/trunk/test/DebugInfo/piece-verifier.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/piece-verifier.ll?rev=226627&view=auto
>> ==============================================================================
>> --- llvm/trunk/test/DebugInfo/piece-verifier.ll (added)
>> +++ llvm/trunk/test/DebugInfo/piece-verifier.ll Tue Jan 20 18:59:20 2015
>> @@ -0,0 +1,54 @@
>> +; RUN: not llvm-as -disable-output -verify-debug-info < %s 2>&1 | FileCheck %s
>> +target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
>> +target triple = "x86_64-apple-macosx10.9.0"
>> +
>> +; Function Attrs: nounwind ssp uwtable
>> +define i32 @foo(i64 %s.coerce0, i32 %s.coerce1) #0 {
>> +entry:
>> + call void @llvm.dbg.value(metadata i64 %s.coerce0, i64 0, metadata !20, metadata !24), !dbg !21
>> + call void @llvm.dbg.value(metadata i32 %s.coerce1, i64 0, metadata !22, metadata !27), !dbg !21
>> + ret i32 %s.coerce1, !dbg !23
>> +}
>> +
>> +; Function Attrs: nounwind readnone
>> +declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
>> +
>> +; Function Attrs: nounwind readnone
>> +declare void @llvm.dbg.value(metadata, i64, metadata, metadata) #1
>> +
>> +attributes #0 = { nounwind ssp uwtable "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" }
>> +attributes #1 = { nounwind readnone }
>> +
>> +!llvm.dbg.cu = !{!0}
>> +!llvm.module.flags = !{!17, !18}
>> +!llvm.ident = !{!19}
>> +
>> +!0 = !{!"0x11\0012\00clang version 3.5 \001\00\000\00\001", !1, !2, !2, !3, !2, !2} ; [ DW_TAG_compile_unit ]
>> +!1 = !{!"pieces.c", !""}
>> +!2 = !{}
>> +!3 = !{!4}
>> +!4 = !{!"0x2e\00foo\00foo\00\003\000\001\000\006\00256\001\003", !1, !5, !6, null, i32 (i64, i32)* @foo, null, null, !15} ; [ DW_TAG_subprogram ] [line 3] [def] [foo]
>> +!5 = !{!"0x29", !1} ; [ DW_TAG_file_type ] [/pieces.c]
>> +!6 = !{!"0x15\00\000\000\000\000\000\000", i32 0, null, null, !7, null, null, null} ; [ DW_TAG_subroutine_type ] [line 0, size 0, align 0, offset 0] [from ]
>> +!7 = !{!8, !9}
>> +!8 = !{!"0x24\00int\000\0032\0032\000\000\005", null, null} ; [ DW_TAG_base_type ] [int] [line 0, size 32, align 32, offset 0, enc DW_ATE_signed]
>> +!9 = !{!"0x16\00S\001\000\000\000\000", !1, null, !10} ; [ DW_TAG_typedef ] [S] [line 1, size 0, align 0, offset 0] [from ]
>> +!10 = !{!"0x13\00\001\00128\0064\000\000\000", !1, null, null, !11, null, null, null} ; [ DW_TAG_structure_type ] [line 1, size 128, align 64, offset 0] [def] [from ]
>> +!11 = !{!12, !14}
>> +!12 = !{!"0xd\00a\001\0064\0064\000\000", !1, !10, !13} ; [ DW_TAG_member ] [a] [line 1, size 64, align 64, offset 0] [from long int]
>> +!13 = !{!"0x24\00long int\000\0064\0064\000\000\005", null, null} ; [ DW_TAG_base_type ] [long int] [line 0, size 64, align 64, offset 0, enc DW_ATE_signed]
>> +!14 = !{!"0xd\00b\001\0032\0032\0064\000", !1, !10, !8} ; [ DW_TAG_member ] [b] [line 1, size 32, align 32, offset 64] [from int]
>> +!15 = !{!16}
>> +!16 = !{!"0x101\00s\0016777219\000", !4, !5, !9} ; [ DW_TAG_arg_variable ] [s] [line 3]
>> +!17 = !{i32 2, !"Dwarf Version", i32 4}
>> +!18 = !{i32 1, !"Debug Info Version", i32 2}
>> +!19 = !{!"clang version 3.5 "}
>> +!20 = !{!"0x101\00s\0016777219\000", !4, !5, !9} ; [ DW_TAG_arg_variable ] [s] [line 3]
>> +!21 = !MDLocation(line: 3, scope: !4)
>> +!22 = !{!"0x101\00s\0016777219\000", !4, !5, !9} ; [ DW_TAG_arg_variable ] [s] [line 3]
>> +!23 = !MDLocation(line: 4, scope: !4)
>> +!24 = !{!"0x102\006\00147\000\008"} ; [ DW_TAG_expression ] [DW_OP_piece 0 8] [piece, size 8, offset 0]
>> +!25 = !{}
>> +; This expression has elements after DW_OP_piece.
>> +; CHECK: DIExpression does not Verify
>> +!27 = !{!"0x102\00147\008\004\006"} ; [ DW_TAG_expression ] [DW_OP_piece 8 4] [piece, size 4, offset 8]
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list