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