[llvm] r340583 - DebugInfo: Improve debug location merging
Adrian Prantl via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 24 15:09:04 PDT 2018
The problem is that scope: can't be a DIFile. It must point to a DILocalScope (DISubprogram, DILexicalBlock).
> On Aug 24, 2018, at 3:07 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
> Not sure I'm following - the example there looks fine (I might be failing to spot the abnormality) - inlinedAt should lead to another DILocation with another scope (& maybe inlinedAt) chain.
>
>
> On Fri, Aug 24, 2018 at 3:05 PM Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
> Unfortunately I don't have a reduced testcase yet, but basically what happens is that
>
> !1 = !DISubprogram(..., file: !0)
> !2 = !DISubprogram(..., file: !0)
> LocA = !DILocation(line: 1, scope: DISubprogram !1, inlinedAt: !3)
> LocB = !DILocation(line: 2, scope: DISubprogram !2, inlinedAt: !3)
>
> and getMergedLocation(LocA, LocB) returns !DILocation(line: 0, scope: !0, inlinedAt: !3), which is invalid.
>
> Lacking a better testcase, I guess we could easily add a unit test for that scenario.
>
> -- adrian
>
>
>> On Aug 24, 2018, at 1:47 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>>
>> No worries - always appreciate the follow-up! Happy to help if it turns out there's anything I can.
>>
>> On Fri, Aug 24, 2018 at 1:17 PM Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
>> Yeah, the Verifier doesn't verify that the scope of a DILocation is a DILocalScope. I'll start there!
>>
>> sorry for the noise.
>> -- adrian
>>
>> > On Aug 24, 2018, at 1:12 PM, Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
>> >
>> > There'a also a >50% chance that this just exposed a bug in the Swift frontend; all reproducers I've got so far are Swift code.
>> >
>> > -- adrian
>> >
>> >> On Aug 24, 2018, at 1:10 PM, Adrian Prantl <aprantl at apple.com <mailto:aprantl at apple.com>> wrote:
>> >>
>> >> I think this commit may be to cause for some failures we are seeing on our internal bots.
>> >> They are hitting an assertion in llvm::DILocation::getMergedLocation(llvm::DILocation const*, llvm::DILocation const*) at
>> >> DIScope *S = LocA->getScope()
>> >> because the scope of LocA is a DIFile (which is not a valid DILocalScope).
>> >>
>> >> I'll post an update once I know more.
>> >>
>> >> -- adrian
>> >>
>> >>> On Aug 23, 2018, at 3:35 PM, David Blaikie via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>> >>>
>> >>> Author: dblaikie
>> >>> Date: Thu Aug 23 15:35:58 2018
>> >>> New Revision: 340583
>> >>>
>> >>> URL: http://llvm.org/viewvc/llvm-project?rev=340583&view=rev <http://llvm.org/viewvc/llvm-project?rev=340583&view=rev>
>> >>> Log:
>> >>> DebugInfo: Improve debug location merging
>> >>>
>> >>> Fix a set of related bugs:
>> >>>
>> >>> * Considering two locations as equivalent when their lines are the same
>> >>> but their scopes are different causes erroneous debug info that
>> >>> attributes a commoned call to be attributed to one of the two calls it
>> >>> was commoned from.
>> >>>
>> >>> * The previous code to compute a new location's scope was inaccurate and
>> >>> would use the inlinedAt that was the /parent/ of the inlinedAt that is
>> >>> the nearest common one, and also used that parent scope instead of the
>> >>> nearest common scope.
>> >>>
>> >>> * Not generating new locations generally seemed like a lower quality
>> >>> choice
>> >>>
>> >>> There was some risk that generating more new locations could hurt object
>> >>> size by making more fine grained line table entries, but it looks like
>> >>> that was offset by the decrease in line table (& address & ranges) size
>> >>> caused by more accurately computing the scope - which likely lead to
>> >>> fewer range entries (more contiguous ranges) & reduced size that way.
>> >>>
>> >>> All up with these changes I saw minor reductions (-1.21%, -1.77%) in
>> >>> .rela.debug_ranges and .rela.debug_addr (in a fission, compressed debug
>> >>> info build) as well as other minor size changes (generally reductinos)
>> >>> across the board (-1.32% debug_info.dwo, -1.28% debug_loc.dwo). Measured
>> >>> in an optimized (-O2) build of the clang binary.
>> >>>
>> >>> If you are investigating a size regression in an optimized debug builds,
>> >>> this is certainly a patch to look into - and I'd be happy to look into
>> >>> any major regressions found & see what we can do to address them.
>> >>>
>> >>> Added:
>> >>> llvm/trunk/test/DebugInfo/X86/merge_inlined_loc.ll
>> >>> Modified:
>> >>> llvm/trunk/include/llvm/IR/DebugInfoMetadata.h
>> >>> llvm/trunk/lib/IR/DebugInfo.cpp
>> >>> llvm/trunk/lib/IR/DebugInfoMetadata.cpp
>> >>> llvm/trunk/test/DebugInfo/COFF/local-variables.ll
>> >>>
>> >>> Modified: llvm/trunk/include/llvm/IR/DebugInfoMetadata.h
>> >>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DebugInfoMetadata.h?rev=340583&r1=340582&r2=340583&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DebugInfoMetadata.h?rev=340583&r1=340582&r2=340583&view=diff>
>> >>> ==============================================================================
>> >>> --- llvm/trunk/include/llvm/IR/DebugInfoMetadata.h (original)
>> >>> +++ llvm/trunk/include/llvm/IR/DebugInfoMetadata.h Thu Aug 23 15:35:58 2018
>> >>> @@ -1479,19 +1479,6 @@ public:
>> >>> return getScope();
>> >>> }
>> >>>
>> >>> - /// Check whether this can be discriminated from another location.
>> >>> - ///
>> >>> - /// Check \c this can be discriminated from \c RHS in a linetable entry.
>> >>> - /// Scope and inlined-at chains are not recorded in the linetable, so they
>> >>> - /// cannot be used to distinguish basic blocks.
>> >>> - bool canDiscriminate(const DILocation &RHS) const {
>> >>> - return getLine() != RHS.getLine() ||
>> >>> - getColumn() != RHS.getColumn() ||
>> >>> - getDiscriminator() != RHS.getDiscriminator() ||
>> >>> - getFilename() != RHS.getFilename() ||
>> >>> - getDirectory() != RHS.getDirectory();
>> >>> - }
>> >>> -
>> >>> /// Get the DWARF discriminator.
>> >>> ///
>> >>> /// DWARF discriminators distinguish identical file locations between
>> >>> @@ -1539,8 +1526,6 @@ public:
>> >>> /// discriminator.
>> >>> inline const DILocation *cloneWithDuplicationFactor(unsigned DF) const;
>> >>>
>> >>> - enum { NoGeneratedLocation = false, WithGeneratedLocation = true };
>> >>> -
>> >>> /// When two instructions are combined into a single instruction we also
>> >>> /// need to combine the original locations into a single location.
>> >>> ///
>> >>> @@ -1555,9 +1540,8 @@ public:
>> >>> ///
>> >>> /// \p GenerateLocation: Whether the merged location can be generated when
>> >>> /// \p LocA and \p LocB differ.
>> >>> - static const DILocation *
>> >>> - getMergedLocation(const DILocation *LocA, const DILocation *LocB,
>> >>> - bool GenerateLocation = NoGeneratedLocation);
>> >>> + static const DILocation *getMergedLocation(const DILocation *LocA,
>> >>> + const DILocation *LocB);
>> >>>
>> >>> /// Returns the base discriminator for a given encoded discriminator \p D.
>> >>> static unsigned getBaseDiscriminatorFromDiscriminator(unsigned D) {
>> >>>
>> >>> Modified: llvm/trunk/lib/IR/DebugInfo.cpp
>> >>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=340583&r1=340582&r2=340583&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfo.cpp?rev=340583&r1=340582&r2=340583&view=diff>
>> >>> ==============================================================================
>> >>> --- llvm/trunk/lib/IR/DebugInfo.cpp (original)
>> >>> +++ llvm/trunk/lib/IR/DebugInfo.cpp Thu Aug 23 15:35:58 2018
>> >>> @@ -690,8 +690,7 @@ unsigned llvm::getDebugMetadataVersionFr
>> >>>
>> >>> void Instruction::applyMergedLocation(const DILocation *LocA,
>> >>> const DILocation *LocB) {
>> >>> - setDebugLoc(DILocation::getMergedLocation(LocA, LocB,
>> >>> - DILocation::WithGeneratedLocation));
>> >>> + setDebugLoc(DILocation::getMergedLocation(LocA, LocB));
>> >>> }
>> >>>
>> >>> //===----------------------------------------------------------------------===//
>> >>>
>> >>> Modified: llvm/trunk/lib/IR/DebugInfoMetadata.cpp
>> >>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfoMetadata.cpp?rev=340583&r1=340582&r2=340583&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugInfoMetadata.cpp?rev=340583&r1=340582&r2=340583&view=diff>
>> >>> ==============================================================================
>> >>> --- llvm/trunk/lib/IR/DebugInfoMetadata.cpp (original)
>> >>> +++ llvm/trunk/lib/IR/DebugInfoMetadata.cpp Thu Aug 23 15:35:58 2018
>> >>> @@ -14,7 +14,7 @@
>> >>> #include "llvm/IR/DebugInfoMetadata.h"
>> >>> #include "LLVMContextImpl.h"
>> >>> #include "MetadataImpl.h"
>> >>> -#include "llvm/ADT/SmallPtrSet.h"
>> >>> +#include "llvm/ADT/SmallSet.h"
>> >>> #include "llvm/ADT/StringSwitch.h"
>> >>> #include "llvm/IR/DIBuilder.h"
>> >>> #include "llvm/IR/Function.h"
>> >>> @@ -69,28 +69,40 @@ DILocation *DILocation::getImpl(LLVMCont
>> >>> }
>> >>>
>> >>> const DILocation *DILocation::getMergedLocation(const DILocation *LocA,
>> >>> - const DILocation *LocB,
>> >>> - bool GenerateLocation) {
>> >>> + const DILocation *LocB) {
>> >>> if (!LocA || !LocB)
>> >>> return nullptr;
>> >>>
>> >>> - if (LocA == LocB || !LocA->canDiscriminate(*LocB))
>> >>> + if (LocA == LocB)
>> >>> return LocA;
>> >>>
>> >>> - if (!GenerateLocation)
>> >>> - return nullptr;
>> >>> -
>> >>> SmallPtrSet<DILocation *, 5> InlinedLocationsA;
>> >>> for (DILocation *L = LocA->getInlinedAt(); L; L = L->getInlinedAt())
>> >>> InlinedLocationsA.insert(L);
>> >>> + SmallSet<std::pair<DIScope *, DILocation *>, 5> Locations;
>> >>> + DIScope *S = LocA->getScope();
>> >>> + DILocation *L = LocA->getInlinedAt();
>> >>> + while (S) {
>> >>> + Locations.insert(std::make_pair(S, L));
>> >>> + S = S->getScope().resolve();
>> >>> + if (!S && L) {
>> >>> + S = L->getScope();
>> >>> + L = L->getInlinedAt();
>> >>> + }
>> >>> + }
>> >>> const DILocation *Result = LocB;
>> >>> - for (DILocation *L = LocB->getInlinedAt(); L; L = L->getInlinedAt()) {
>> >>> - Result = L;
>> >>> - if (InlinedLocationsA.count(L))
>> >>> + S = LocB->getScope();
>> >>> + L = LocB->getInlinedAt();
>> >>> + while (S) {
>> >>> + if (Locations.count(std::make_pair(S, L)))
>> >>> break;
>> >>> + S = S->getScope().resolve();
>> >>> + if (!S && L) {
>> >>> + S = L->getScope();
>> >>> + L = L->getInlinedAt();
>> >>> + }
>> >>> }
>> >>> - return DILocation::get(Result->getContext(), 0, 0, Result->getScope(),
>> >>> - Result->getInlinedAt());
>> >>> + return DILocation::get(Result->getContext(), 0, 0, S, L);
>> >>> }
>> >>>
>> >>> DINode::DIFlags DINode::getFlag(StringRef Flag) {
>> >>>
>> >>> Modified: llvm/trunk/test/DebugInfo/COFF/local-variables.ll
>> >>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/COFF/local-variables.ll?rev=340583&r1=340582&r2=340583&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/COFF/local-variables.ll?rev=340583&r1=340582&r2=340583&view=diff>
>> >>> ==============================================================================
>> >>> --- llvm/trunk/test/DebugInfo/COFF/local-variables.ll (original)
>> >>> +++ llvm/trunk/test/DebugInfo/COFF/local-variables.ll Thu Aug 23 15:35:58 2018
>> >>> @@ -60,7 +60,7 @@
>> >>> ; ASM: leaq 36(%rsp), %rcx
>> >>> ; ASM: [[else_end:\.Ltmp.*]]:
>> >>> ; ASM: .LBB0_3: # %if.end
>> >>> -; ASM: .cv_loc 0 1 17 1 # t.cpp:17:1
>> >>> +; ASM: .cv_loc 0 1 0 0 # t.cpp:0:0
>> >>> ; ASM: callq capture
>> >>> ; ASM: nop
>> >>> ; ASM: addq $56, %rsp
>> >>>
>> >>> Added: llvm/trunk/test/DebugInfo/X86/merge_inlined_loc.ll
>> >>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/X86/merge_inlined_loc.ll?rev=340583&view=auto <http://llvm.org/viewvc/llvm-project/llvm/trunk/test/DebugInfo/X86/merge_inlined_loc.ll?rev=340583&view=auto>
>> >>> ==============================================================================
>> >>> --- llvm/trunk/test/DebugInfo/X86/merge_inlined_loc.ll (added)
>> >>> +++ llvm/trunk/test/DebugInfo/X86/merge_inlined_loc.ll Thu Aug 23 15:35:58 2018
>> >>> @@ -0,0 +1,100 @@
>> >>> +; RUN: llc %s -mtriple=x86_64-unknown-unknown -o - | FileCheck %s
>> >>> +
>> >>> +; Generated with "clang -g -c -emit-llvm -S -O3"
>> >>> +
>> >>> +; This will test several features of merging debug locations. Importantly,
>> >>> +; locations with the same source line but different scopes should be merged to
>> >>> +; a line zero location at the nearest common scope and inlining. The location
>> >>> +; of the single call to "common" (the two calls are collapsed together by
>> >>> +; BranchFolding) should be attributed to line zero inside the wrapper2 inlined
>> >>> +; scope within f1.
>> >>> +
>> >>> +; void common();
>> >>> +; inline void wrapper() { common(); }
>> >>> +; extern bool b;
>> >>> +; void sink();
>> >>> +; inline void wrapper2() {
>> >>> +; if (b) {
>> >>> +; sink();
>> >>> +; wrapper();
>> >>> +; } else
>> >>> +; wrapper();
>> >>> +; }
>> >>> +; void f1() { wrapper2(); }
>> >>> +
>> >>> +; Ensure there is only one inlined_subroutine (for wrapper2, none for wrapper)
>> >>> +; & that its address range includes the call to 'common'.
>> >>> +
>> >>> +; CHECK: jmp _Z6commonv
>> >>> +; CHECK-NEXT: [[LABEL:.*]]:
>> >>> +
>> >>> +; CHECK: .section .debug_info
>> >>> +; CHECK: DW_TAG_subprogram
>> >>> +; CHECK: DW_TAG_subprogram
>> >>> +; CHECK-NOT: {{DW_TAG\|End Of Children}}
>> >>> +; CHECK: DW_TAG_inlined_subroutine
>> >>> +; CHECK-NOT: {{DW_TAG\|End Of Children}}
>> >>> +; CHECK: [[LABEL]]-{{.*}} DW_AT_high_pc
>> >>> +; CHECK-NOT: DW_TAG
>> >>> +
>> >>> +
>> >>> +
>> >>> + at b = external dso_local local_unnamed_addr global i8, align 1
>> >>> +
>> >>> +; Function Attrs: uwtable
>> >>> +define dso_local void @_Z2f1v() local_unnamed_addr !dbg !7 {
>> >>> +entry:
>> >>> + %0 = load i8, i8* @b, align 1, !dbg !10, !tbaa !14, !range !18
>> >>> + %tobool.i = icmp eq i8 %0, 0, !dbg !10
>> >>> + br i1 %tobool.i, label %if.else.i, label %if.then.i, !dbg !19
>> >>> +
>> >>> +if.then.i: ; preds = %entry
>> >>> + tail call void @_Z4sinkv(), !dbg !20
>> >>> + tail call void @_Z6commonv(), !dbg !22
>> >>> + br label %_Z8wrapper2v.exit, !dbg !25
>> >>> +
>> >>> +if.else.i: ; preds = %entry
>> >>> + tail call void @_Z6commonv(), !dbg !26
>> >>> + br label %_Z8wrapper2v.exit
>> >>> +
>> >>> +_Z8wrapper2v.exit: ; preds = %if.then.i, %if.else.i
>> >>> + ret void, !dbg !28
>> >>> +}
>> >>> +
>> >>> +declare dso_local void @_Z4sinkv() local_unnamed_addr
>> >>> +
>> >>> +declare dso_local void @_Z6commonv() local_unnamed_addr
>> >>> +
>> >>> +!llvm.dbg.cu <http://llvm.dbg.cu/> = !{!0}
>> >>> +!llvm.module.flags = !{!3, !4, !5}
>> >>> +!llvm.ident = !{!6}
>> >>> +
>> >>> +!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 8.0.0 (trunk 340559) (llvm/trunk 340572)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
>> >>> +!1 = !DIFile(filename: "merge_loc.cpp", directory: "/usr/local/google/home/blaikie/dev/scratch")
>> >>> +!2 = !{}
>> >>> +!3 = !{i32 2, !"Dwarf Version", i32 4}
>> >>> +!4 = !{i32 2, !"Debug Info Version", i32 3}
>> >>> +!5 = !{i32 1, !"wchar_size", i32 4}
>> >>> +!6 = !{!"clang version 8.0.0 (trunk 340559) (llvm/trunk 340572)"}
>> >>> +!7 = distinct !DISubprogram(name: "f1", linkageName: "_Z2f1v", scope: !1, file: !1, line: 12, type: !8, isLocal: false, isDefinition: true, scopeLine: 12, flags: DIFlagPrototyped, isOptimized: true, unit: !0, retainedNodes: !2)
>> >>> +!8 = !DISubroutineType(types: !9)
>> >>> +!9 = !{null}
>> >>> +!10 = !DILocation(line: 6, column: 7, scope: !11, inlinedAt: !13)
>> >>> +!11 = distinct !DILexicalBlock(scope: !12, file: !1, line: 6, column: 7)
>> >>> +!12 = distinct !DISubprogram(name: "wrapper2", linkageName: "_Z8wrapper2v", scope: !1, file: !1, line: 5, type: !8, isLocal: false, isDefinition: true, scopeLine: 5, flags: DIFlagPrototyped, isOptimized: true, unit: !0, retainedNodes: !2)
>> >>> +!13 = distinct !DILocation(line: 13, column: 3, scope: !7)
>> >>> +!14 = !{!15, !15, i64 0}
>> >>> +!15 = !{!"bool", !16, i64 0}
>> >>> +!16 = !{!"omnipotent char", !17, i64 0}
>> >>> +!17 = !{!"Simple C++ TBAA"}
>> >>> +!18 = !{i8 0, i8 2}
>> >>> +!19 = !DILocation(line: 6, column: 7, scope: !12, inlinedAt: !13)
>> >>> +!20 = !DILocation(line: 7, column: 5, scope: !21, inlinedAt: !13)
>> >>> +!21 = distinct !DILexicalBlock(scope: !11, file: !1, line: 6, column: 10)
>> >>> +!22 = !DILocation(line: 2, column: 25, scope: !23, inlinedAt: !24)
>> >>> +!23 = distinct !DISubprogram(name: "wrapper", linkageName: "_Z7wrapperv", scope: !1, file: !1, line: 2, type: !8, isLocal: false, isDefinition: true, scopeLine: 2, flags: DIFlagPrototyped, isOptimized: true, unit: !0, retainedNodes: !2)
>> >>> +!24 = distinct !DILocation(line: 8, column: 5, scope: !21, inlinedAt: !13)
>> >>> +!25 = !DILocation(line: 9, column: 3, scope: !21, inlinedAt: !13)
>> >>> +!26 = !DILocation(line: 2, column: 25, scope: !23, inlinedAt: !27)
>> >>> +!27 = distinct !DILocation(line: 10, column: 5, scope: !11, inlinedAt: !13)
>> >>> +!28 = !DILocation(line: 14, column: 1, scope: !7)
>> >>>
>> >>>
>> >>> _______________________________________________
>> >>> llvm-commits mailing list
>> >>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>> >>
>> >
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180824/eccded62/attachment.html>
More information about the llvm-commits
mailing list