In D64544#1589926, @JDevlieghere wrote:

In D64544#1589861, @probinson wrote:

In D64544#1589853, @dblaikie wrote:

I was rather hoping for test coverage for all the new error messages this change introduced - is that unrealistic/impractical?

Oh, sure, I was focussed on the MD5 one because that's the only one that's "new". I'll make sure the other ones are covered too.

Yeah, the line table is especially tricky to hand-craft compared to checking in an object file. I think it technically can still be hand-crafted assembly (no line directives, etc, just a debug_line section with raw byte (etc) directives) - might be plausible & make it clearer what the input is? (checked in assembly, assembled with llvm-mc then run through llvm-dwarfdump to test the parsing)

Hear hear. It looks like this patch is all about diagnosing issues in the v5 prologue/file-table, and it should be relatively easy to construct bogus examples in assembler. You don't need a line-number program at all. There ought to be examples of *valid* v5 line-table headers lying around, I know I did all the dumper work with assembler tests before I ever generated any v5 headers.

How would you be able to emit a wrong form value for the MD5 hash from assembly? The directive (which I think you came up with?) is just md5 [data]? Deciding which form to emit is hard-coded in MCDwarf.

I was rather hoping for test coverage for all the new error messages this change introduced - is that unrealistic/impractical?

Test coverage? (you could add another inlined call to the test case you added for the feature - but just leave the inlined-at location with a zero column for that particular call - so you can still test both positive/negatiev in the one file)

In D64033#1586885, @wenlei wrote:

In D64033#1586625, @dblaikie wrote:

What's the motivation for this?

We need to differentiate inline sites on the same line for an internal (basically the tool needs to annotate each instruction with stack of inline site locations). And the missing column info for inline sites gets in the way. I thought the change will be helpful to others as well..

What's the motivation for this?
Should it be conditional on "-gcolumn-info"? (or skipping it in general if the column is zero? that'd make "-gno-column-info" fall out naturally)
(we don't put decl_column on subprogram DIEs, for instance - admittedly, it's more likely the same function could have multiple inline call sites on the same line than you'd have multiple functions defined on the same line)

Looks like this code already had rudimentary error handling along this codepath where the assert is (the function returns empty, which returns false, which becomes an error further up, I think?) - and adding more descriptive/precise error handling along this path is probably good - but probably also means more testing is required to demonstrate all the new error results/messages/codepaths?

Sounds good!

In D64601#1583538, @asbirlea wrote:

I don't know if a bot exists with LLVM_ENABLE_REVERSE_ITERATION set to true, but if it does, then the check I proposed for the uselist order would likely fail, no?

In D64601#1583240, @asbirlea wrote:

Hmm, AFAICT, there is no opt flag equivalent for LLVM_ENABLE_REVERSE_ITERATION. Did I miss it?

In D64601#1581770, @asbirlea wrote:

You can use SetVector's "getArrayRef" and pass it as "void removeBlocks(ArrayRef<BasicBlock *> DeadBlocks);"

removeBlocks is querying DeadBlocks with count(...). I can change the code to walk the Vector/ArrayRef, but it defeats the purpose of building a set.
It may be ok to change all callsites to just use vector too, assuming the normal expected size is small, but it may be costly for pathological cases (large number of deadblocks).
If folks have a preference either way, please let me know.

In D64601#1581711, @asbirlea wrote:

I'd be interested if there's a better way to use a SmallSetVector, without needing the "8". I don't see an Impl version.
The only alternative I came up with was something like:

template <class OrderedSetT>
void removeBlocks(const OrderedSetT &DeadBlocks);

Ping

In D61584#1579992, @HsiangKai wrote:

In D61584#1579871, @dblaikie wrote:

In D61584#1579860, @HsiangKai wrote:

In D61584#1574203, @dblaikie wrote:

Does this solution extend/address other label deltas, like those that would appear in the other DWARF/non-frame sections? (eg: currently we use a length in the DWARF high_pc because it means not having to use a relocation for high_pc, which reduces object size - so, does this fix cause relocations to be used for that length expression to ensure they're correct if the linker changes the instructions in that range?)

After D45181 committed, LLVM will generate relocation pairs for symbol difference expressions if target enables relaxation. So, it will generate relocations for high_pc due to it is a symbol difference expression of end symbol and start symbol of the function range. Such as,

.word   .Lfunc_begin0           # DW_AT_low_pc
.word   .Lfunc_end4-.Lfunc_begin0 # DW_AT_high_pc

This commit modified the condition to generate relocations for symbol difference expressions. Not all symbol difference expressions are related to executable code. However, only executable code will be modified by linker for relaxation. So, we do not need to generate relocations for symbol differences not located in executable code sections.

Good to know - so has LLVM been creating incorrect DWARF in these cases so far - or has no target enabled linker relaxation yet?

Is there any way to make this more targeted - does linker relaxation allow arbitrary machine code changes? Or is there something more fine-grained - so we could compute compile-time constants for some label differences, but only use linker relocations for cases where we know the instruction sequence includes something relaxable?

AFAIK only RISC-V enables this feature. Currently, .debug_frame/.eh_frame is wrong after relaxation. I already fixed parts of the problem in D58335. In D58335, .debug_frame/.eh_frame will generate relocations if relaxation is enabled. Otherwise, it will not aware relaxation at all and it doesn't work to do frame inspection when debugging.

Linker relaxation processes some patterns of instruction sequences. For example, RISC-V could use auipc/jalr to jump to 32-bits pc-relative address. If linker knows the jump target is within +/-1 Mib, it could replace auipc/jalr to jal. The replacement will shorten the instruction sequence. So, the label differences between these instruction sequences may be changed after relaxation. That is why we need to annotate relocation types for these symbol differences in code sections.

Currently, there is no way to make it more targeted. When relaxation is enabled, it assumes all code sequences may be changed after relaxation. If we want to make placement of relocations more specific, we need to inspect every instruction between two symbols when we create symbol difference expressions for them. I think it will do a lot of work to gain few benefits. (Fewer relocations, smaller object code, less linker efforts, etc.)

In D61584#1579860, @HsiangKai wrote:

In D61584#1574203, @dblaikie wrote:

Does this solution extend/address other label deltas, like those that would appear in the other DWARF/non-frame sections? (eg: currently we use a length in the DWARF high_pc because it means not having to use a relocation for high_pc, which reduces object size - so, does this fix cause relocations to be used for that length expression to ensure they're correct if the linker changes the instructions in that range?)

After D45181 committed, LLVM will generate relocation pairs for symbol difference expressions if target enables relaxation. So, it will generate relocations for high_pc due to it is a symbol difference expression of end symbol and start symbol of the function range. Such as,

.word   .Lfunc_begin0           # DW_AT_low_pc
.word   .Lfunc_end4-.Lfunc_begin0 # DW_AT_high_pc

This commit modified the condition to generate relocations for symbol difference expressions. Not all symbol difference expressions are related to executable code. However, only executable code will be modified by linker for relaxation. So, we do not need to generate relocations for symbol differences not located in executable code sections.

Looks good :)

I would've assumed these conditionals were added by Sony folks for their change in default dialect - that doesn't necessarily mean these tests are needed upstream (the functionality may be tested elsewhere)

Does this solution extend/address other label deltas, like those that would appear in the other DWARF/non-frame sections? (eg: currently we use a length in the DWARF high_pc because it means not having to use a relocation for high_pc, which reduces object size - so, does this fix cause relocations to be used for that length expression to ensure they're correct if the linker changes the instructions in that range?)

Looks good to me. (possible the check (for no default capture and no captures) could be unified with the same check/language used for the implicit function pointer conversion operator ("if (Captures.empty() && CaptureDefault == LCD_None)" - around 1744 in SemaLambda.cpp) - maybe a common utility function to test this condition or the like. But it's hardly a big thing to have it written in two places, really)

In D64006#1568874, @ikudrin wrote:

In D64006#1568562, @dblaikie wrote:

I think the main one is code duplication - write once/fix once/etc is valuable.

In that case, what do you think about templates?

In D63672#1568862, @diegotf wrote:

In D63672#1568616, @dblaikie wrote:

In D63672#1567614, @diegotf wrote:

In D63672#1565680, @dblaikie wrote:

Did the broader design discussion about bugpoint lead to this design (a separate llvm-reduce tool)? I didn't quite follow that thread far enough to see the conclusion that lead to this direction (rather than improving/modifying bugpoint) - might be worth writing a bit more in the commit description about the conversation (link to the thread if that's helpful) and design choices - and long-term plan (to merge behavior, share things, remove one or the other - or should these two tools exist indefinitely with different goals/uses?)

Thanks for the comment, I've clarified it in the description. As well as a brief description of how the tool is structured.

The intent is to build this separately, then fold it into bugpoint? What's the tradeoff/motivation for that compared to building this into bugpoint to begin with? (seems like it presents a risk that that unification doesn't happen & we're left with a fractured solution ('one more competing standard' sort of situation)) Is the desire to get this reviewed/committed sooner while pursuing some bugpoint refactoring necessary before unification, without delaying implementation of this tool until after that refactoring?

The main reason I chose this route was to not interfere with bugpoint's current users, especially because there's low test coverage, these changes might end up doing more harm than good. Additionally, some community members also voiced their concerns about how the redesign might affect their current workflow.

I also agree the risks you mentioned are very real indeed, and I'm not still 100% sure this might be the best approach so other routes might be worth exploring.

In D63672#1567614, @diegotf wrote:

In D63672#1565680, @dblaikie wrote:

Did the broader design discussion about bugpoint lead to this design (a separate llvm-reduce tool)? I didn't quite follow that thread far enough to see the conclusion that lead to this direction (rather than improving/modifying bugpoint) - might be worth writing a bit more in the commit description about the conversation (link to the thread if that's helpful) and design choices - and long-term plan (to merge behavior, share things, remove one or the other - or should these two tools exist indefinitely with different goals/uses?)

Thanks for the comment, I've clarified it in the description. As well as a brief description of how the tool is structured.

In D64006#1567868, @ikudrin wrote:

In D64006#1566831, @aprantl wrote:

...in places where it likely won't be needed any time soon, such as 64-bit DWARF parsing support.

In fact, I am going to implement 64-bit DWARF support. I started from DataExtractor because changing it seems inevitable for that anyway.

In D64006#1567394, @dblaikie wrote:

But it sounds like @ikudrin is suggesting maybe it's not their goal to migrate from 32 to 64 entirely, and that maybe it's OK to have both APIs?

In the beginning, I also thought that having only one possible type for offsets, uint64_t, is preferable. But thinking of that for some time, I cannot find any strong reason not to keep both variants, apart from some kind of aesthetic sense, which is debatable. I really do not see any new harm which may result from having overloads for both 32- and 64-bit offsets. The only issue I can guess is a potential risk of wrapping a 32-bit offset value, but the current implementation is also affected by that, so nothing new is here.

In D64006#1566831, @aprantl wrote:

Thank you for demonstrating this! I agree that the patch also introduces lots of opportunities for subtle bugs. It also increases the memory footprint in places where it likely won't be needed any time soon, such as 64-bit DWARF parsing support. This patch looks much more palatable to me now and I'd be okay with taking it if @dblaikie is okay with it. Is there danger in using the same function name or should we call the 64-bit variants something more explicit?

In D63713#1566605, @labath wrote:

Ok, I got some numbers now. I went for a micro-benchmark-like setup to show some kind of a worst case scenario. The test setup is as follows:

I took the largest single .o file I had around (Registry.cpp.o in clangDynamicASTMatchers library). I then linked it to remove any relocations (otherwise, most of the time is spend applying those). Then I modified llvm-dwarfdump to parse the debug_loc section without dumping anything (to avoid measuring the time spend in printfs). Both llvm-dwarfdump and Registry.cpp.o I was dumping were built with -O3 -g, with asserts disabled (no FDO, LTO or other fancy stuff). This resulted in about 4.5 megabytes of debug_loc for parsing in Registry.cpp.o. Then I used the linux perf command to run llvm-dwarfdump -debug-loc 1000 times and dump the stats.

The baseline stats are:

  27.285986      task-clock:u (msec)       #    0.986 CPUs utilized            ( +-  0.11% )
          0      context-switches:u        #    0.000 K/sec                  
          0      cpu-migrations:u          #    0.000 K/sec                  
      2,813      page-faults:u             #    0.103 M/sec                    ( +-  0.24% )
 58,831,163      cycles:u                  #    2.156 GHz                      ( +-  0.07% )
    606,986      stalled-cycles-frontend:u #    1.03% frontend cycles idle     ( +-  3.76% )
  7,924,778      stalled-cycles-backend:u  #   13.47% backend cycles idle      ( +-  0.33% )
146,588,727      instructions:u            #    2.49  insn per cycle         
                                           #    0.05  stalled cycles per insn  ( +-  0.00% )
 29,545,620      branches:u                # 1082.813 M/sec                    ( +-  0.00% )
    222,276      branch-misses:u           #    0.75% of all branches          ( +-  0.15% )
 
0.027663381 seconds time elapsed                                          ( +-  0.11% )

The stats with this patch applied are:

  27.397390      task-clock:u (msec)       #    0.987 CPUs utilized            ( +-  0.10% )
          0      context-switches:u        #    0.000 K/sec                  
          0      cpu-migrations:u          #    0.000 K/sec                  
      2,833      page-faults:u             #    0.103 M/sec                    ( +-  0.24% )
 60,160,571      cycles:u                  #    2.196 GHz                      ( +-  0.07% )
    584,825      stalled-cycles-frontend:u #    0.97% frontend cycles idle     ( +-  3.37% )
 10,729,974      stalled-cycles-backend:u  #   17.84% backend cycles idle      ( +-  0.26% )
156,141,836      instructions:u            #    2.60  insn per cycle         
                                           #    0.07  stalled cycles per insn  ( +-  0.00% )
 31,599,940      branches:u                # 1153.392 M/sec                    ( +-  0.00% )
    221,247      branch-misses:u           #    0.70% of all branches          ( +-  0.06% )
 
0.027771865 seconds time elapsed                                          ( +-  0.10% )

The stats for a version of this patch which additionally checks for the error flag before attempting the parse (as discussed in the inline comment) are:

  27.808349      task-clock:u (msec)       #    0.986 CPUs utilized            ( +-  0.10% )
          0      context-switches:u        #    0.000 K/sec                  
          0      cpu-migrations:u          #    0.000 K/sec                  
      2,839      page-faults:u             #    0.102 M/sec                    ( +-  0.24% )
 62,887,388      cycles:u                  #    2.261 GHz                      ( +-  0.06% )
    575,264      stalled-cycles-frontend:u #    0.91% frontend cycles idle     ( +-  3.18% )
 14,757,888      stalled-cycles-backend:u  #   23.47% backend cycles idle      ( +-  0.23% )
167,562,307      instructions:u            #    2.66  insn per cycle         
                                           #    0.09  stalled cycles per insn  ( +-  0.00% )
 33,414,152      branches:u                # 1201.587 M/sec                    ( +-  0.00% )
    221,454      branch-misses:u           #    0.66% of all branches          ( +-  0.12% )
 
0.028201319 seconds time elapsed                                          ( +-  0.10% )

As can be seen, this patch increases the parsing time by about 0.4%. This is enough to be statistically significant in a benchmark like this, but probably not world-shattering (some slowdown is unavoidable with a change like this).

If we additionally enable the early returns we get an additional 1.5% slowdown (or 1.9% above baseline). Still not bad for a "micro-benchmark", but it does make one wonder, whether it is really worth it. My feeling would be that it isn't...

Did the broader design discussion about bugpoint lead to this design (a separate llvm-reduce tool)? I didn't quite follow that thread far enough to see the conclusion that lead to this direction (rather than improving/modifying bugpoint) - might be worth writing a bit more in the commit description about the conversation (link to the thread if that's helpful) and design choices - and long-term plan (to merge behavior, share things, remove one or the other - or should these two tools exist indefinitely with different goals/uses?)

Yeah, that looks incorrect/meaningless in this context - thanks for the catch!

In D63905#1562040, @rovka wrote:

In D63905#1561649, @dblaikie wrote:

Looks good to me, please commit

(@rovka - this is at least a short-term fix, I'm approving this to unbreak LLVM's mainline - feel free to refix with other ideas if you have any (also happy to discuss this further with you here, IRC, or elsewhere on the mailing lists)

Thanks for looking into it and sorry about the breakage. I think one InVReg is sufficient, there's no need for a whole SmallVector there. What do you think about something like this?

Looks good to me, please commit

Sounds alright :)

In D46628#1558438, @luciang wrote:

I found that clang 9 (and previous versions) also generates non CU info in .debug_info with -gmlt:

$ echo 'struct A { A() {} int a = 0; }; struct B { B(); A a; }; B::B() {}' | clang++ -x c++ -O2 -gmlt -o clang-02-gmlt.o -c - && llvm-dwarfdump --debug-info clang-02-gmlt.o
clang-02-gmlt.o:	file format ELF64-x86-64

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000051 version = 0x0004 abbr_offset = 0x0000 addr_size = 0x08 (next unit at 0x00000055)

0x0000000b: DW_TAG_compile_unit
              DW_AT_producer	("clang version 9.0.0 (https://github.com/llvm/llvm-project 01a99c0aa5ae5be47ea62bd6c87ca6bb63f5a454)")
              DW_AT_language	(DW_LANG_C_plus_plus)
              DW_AT_name	("-")
              DW_AT_stmt_list	(0x00000000)
              DW_AT_comp_dir	("/tmp/test")
              DW_AT_low_pc	(0x0000000000000000)
              DW_AT_high_pc	(0x0000000000000007)

0x0000002a:   DW_TAG_subprogram
                DW_AT_name	("A")

0x0000002f:   DW_TAG_subprogram
                DW_AT_low_pc	(0x0000000000000000)
                DW_AT_high_pc	(0x0000000000000007)
                DW_AT_name	("B")

0x00000040:     DW_TAG_inlined_subroutine
                  DW_AT_abstract_origin	(0x0000002a "A")
                  DW_AT_low_pc	(0x0000000000000000)
                  DW_AT_high_pc	(0x0000000000000006)
                  DW_AT_call_file	("/tmp/test/<stdin>")
                  DW_AT_call_line	(1)

0x00000053:     NULL

0x00000054:   NULL

Note that dropping with -O0 or -O1 .debug_info the DW_TAG_subprogram and DW_TAG_inlined_subroutine DIEs are not generated. -Os, -O2 and -O3 do.

$ echo 'struct A { A() {} int a = 0; }; struct B { B(); A a; }; B::B() {}' | clang++ -x c++ -O0 -gmlt -o clang-02-gmlt.o -c - && llvm-dwarfdump --debug-info clang-02-gmlt.o
clang-02-gmlt.o:	file format ELF64-x86-64

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000026 version = 0x0004 abbr_offset = 0x0000 addr_size = 0x08 (next unit at 0x0000002a)

0x0000000b: DW_TAG_compile_unit
              DW_AT_producer	("clang version 9.0.0 (https://github.com/llvm/llvm-project 01a99c0aa5ae5be47ea62bd6c87ca6bb63f5a454)")
              DW_AT_language	(DW_LANG_C_plus_plus)
              DW_AT_name	("-")
              DW_AT_stmt_list	(0x00000000)
              DW_AT_comp_dir	("/tmp/test")
              DW_AT_low_pc	(0x0000000000000000)
              DW_AT_ranges	(0x00000000
                 [0x0000000000000000, 0x000000000000001b)
                 [0x0000000000000000, 0x0000000000000014))

---

The proposed --strip-debug-non-line is an improvement on top of what current gcc/clang produce with -g1 or -gmlt:

$ echo 'struct A { A() {} int a = 0; }; struct B { B(); A a; }; B::B() {}; int main(){ return 0;}' | clang++ -x c++ -O2 -gmlt -o clang-02-gmlt-strip -fuse-ld=lld -Wl,--strip-debug-non-line - && llvm-dwarfdump --debug-info clang-02-gmlt-strip
clang-02-gmlt-strip:	file format ELF64-x86-64

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000026 version = 0x0004 abbr_offset = 0x0000 addr_size = 0x08 (next unit at 0x0000002a)

0x0000000b: DW_TAG_compile_unit
              DW_AT_producer	("clang version 9.0.0 (https://github.com/llvm/llvm-project 01a99c0aa5ae5be47ea62bd6c87ca6bb63f5a454)")
              DW_AT_language	(DW_LANG_C_plus_plus)
              DW_AT_name	("-")
              DW_AT_stmt_list	(0x00000000)
              DW_AT_comp_dir	("/tmp/test")
              DW_AT_low_pc	(0x0000000000201100)
              DW_AT_high_pc	(0x0000000000201113)

vs

$ echo 'struct A { A() {} int a = 0; }; struct B { B(); A a; }; B::B() {}; int main(){ return 0;}' | clang++ -x c++ -O2 -gmlt -o clang-02-gmlt -fuse-ld=lld  - && llvm-dwarfdump --debug-info clang-02-gmlt
clang-02-gmlt:	file format ELF64-x86-64

.debug_info contents:
0x00000000: Compile Unit: length = 0x00000051 version = 0x0004 abbr_offset = 0x0000 addr_size = 0x08 (next unit at 0x00000055)

0x0000000b: DW_TAG_compile_unit
              DW_AT_producer	("clang version 9.0.0 (https://github.com/llvm/llvm-project 01a99c0aa5ae5be47ea62bd6c87ca6bb63f5a454)")
              DW_AT_language	(DW_LANG_C_plus_plus)
              DW_AT_name	("-")
              DW_AT_stmt_list	(0x00000000)
              DW_AT_comp_dir	("/tmp/test")
              DW_AT_low_pc	(0x0000000000201100)
              DW_AT_high_pc	(0x0000000000201113)

0x0000002a:   DW_TAG_subprogram
                DW_AT_name	("A")

0x0000002f:   DW_TAG_subprogram
                DW_AT_low_pc	(0x0000000000201100)
                DW_AT_high_pc	(0x0000000000201107)
                DW_AT_name	("B")

0x00000040:     DW_TAG_inlined_subroutine
                  DW_AT_abstract_origin	(0x0000002a "A")
                  DW_AT_low_pc	(0x0000000000201100)
                  DW_AT_high_pc	(0x0000000000201106)
                  DW_AT_call_file	("/tmp/test/<stdin>")
                  DW_AT_call_line	(1)

0x00000053:     NULL

0x00000054:   NULL

(I'll look into what triggers -gmlt + -O2 to generate the extra debug sections separately).

In D63537#1557210, @MaskRay wrote:

@dblaikie save gdb-index generates index files of version 8. gold/lld generate version 7, llvm-dwarfdump recognize version 7 but not 8...

mkdir dir
gdb a -batch -q -ex 'save gdb-index dir'
objcopy --add-section .gdb_index=dir/a.gdb-index --set-section-flags .gdb_index=readonly a b

% myllvm-dwarfdump -gdb-index b
b:      file format ELF64-x86-64

.gdb_index contents:

<error parsing>

In D63632#1556532, @rriddle wrote:

My understanding is that input_iterators do not have to return a reference, and may return a value type or some other proxy object.

(Sorry if there is a specific website that should be linked)
See specification of LegacyInputIterator: https://en.cppreference.com/w/cpp/named_req/InputIterator

My understanding was/is that iterators can't return values instead of references (that such a thing doesn't meet the standard requirements for iterators) and that iterators basically have to have internal storage for things like this.

I'm kind of thinking a change to the cursor as Greg suggested might be better than wrapping the stream further - and probably using llvm::Error.

Given figuring out error handling for DataExtractor is perhaps a wider issue - if you want to go ahead with this change (continue with the review & defer error handling improvements for later, leave a FIXME, etc) that seems fine.

Awesome - thanks!

Sounds alright to me

In D62635#1548721, @rnk wrote:

In D62635#1548648, @dblaikie wrote:

In D62635#1548157, @rnk wrote:

We did things this way to track which enumerators were used, not which enums were used. Size data showed it was worth doing (6%). The existing format doesn't support tracking usage of individual enumerators, so we pretended they were const integers to avoid changing the schema.

Ah - describing all the enumerators in any emitted enum would be too many bits/too much size in output?

Yes, that's what @akhuang tried and measured against to come up with the 6% number.

In D62635#1548157, @rnk wrote:

We did things this way to track which enumerators were used, not which enums were used. Size data showed it was worth doing (6%). The existing format doesn't support tracking usage of individual enumerators, so we pretended they were const integers to avoid changing the schema.

In D62635#1547352, @akhuang wrote:

I think the main issue was keeping track of which enums are used?

Is it practical/possible to do this in LLVM rather than in Clang? I'd have thought it'd be best to keep the IR metadata as output-format agnostic as practical/possible to reduce divergent codepaths, etc?

Ping (just curious about the change to the canonicalization printing policy - making sure I don't break something important)

Does the original bug show up with TSan on any existing/checked in tests, or could TSan catch it in some new test that isn't checked in yet/should be checked in?

I don't fully have the direction you've in mind/that this is a part of in my head - but roughly enough to give a thumbs-up here :)

Looks good!

Looks good - thanks!

In D63013#1537212, @aprantl wrote:

dwarfdump(-classic) on macOS also supported the --out-file option and on macOS we strive for compatibility with the older tool.

In D63013#1537125, @aprantl wrote:

In D63013#1536440, @jhenderson wrote:

In D63013#1535260, @aprantl wrote:

I'm mildly against this change. IIRC, the idea was that --out-file is a long-form option that nobody uses in practice and we wanted people to prefer -o instead, which is what people are most familiar with working with compilers. So if -o shows up in the --help output, I'd leave it at that.

Could we just delete --out-file, if we don't want people to use it?

Not any more, I'm afraid :-)
Once an option is out there we will no doubt get complaints that we broke somebody's scripts if we remove it.

Sounds good - thanks

In D62634#1527634, @probinson wrote:

In D62634#1527575, @dblaikie wrote:

This is intentional behavior in clang (controllable under the -f[no-]split-dwarf-inlining flag, and modified by the -f[no-]debug-info-for-profiling flag).

This extra debug info is used for online symbolication (in the absence of .dwo files) - such as for the sanitizers (accurate symbolication is necessary for correctness in msan, due to msan's necessary use of blacklisting to avoid certain false positives) and some forms of sample based profiling collection.

In the default mode, clang includes, as you noted, just the subprograms that have inlined subroutines in them in this split-dwarf-inlining data.
In -fdebug-info-for-profiling all subprograms are described in the skeleton CU with some minimal attributes (they don't need parameters, local variables/scopes, etc) necessary to do certain profile things I don't know lots about.

I think we have to tolerate the msan part. However, the profile feedback workflow could (should) surely be taught to read a .dwo file. The point of -fdebug-info-for-profiling was so you don't have to emit limited/full debug info in the .o file just to make profiling work; but if you're using split DWARF then you're doing limited/full anyway, and the feedback loop happens in the context of a build environment so the .dwo can be asserted to be available. So in split DWARF mode, we should not be emitting debug-info-for-profiling into the skeleton.

In D62369#1527350, @jhenderson wrote:

@dblaikie, did this get committed? I think @mmpozulp you may now have access so could commit if it hasn't happened yet.

In D62634#1527533, @labath wrote:

Actually, there was a subtle change of behavior here. Before this change, if we tried to parse a DIE using an abbreviation table from a different file, we would most likely fail immediately because of the fail-safe in GetAbbreviationDeclarationPtr. Now, we just do a blind read and return bogus data.

Now, normally this shouldn't matter, but in case of split-dwarf, things start to get interesting. Lldb has this assumption that when we are reading a debug info entry (which is not the CU DIE), we are actually reading it from the dwo compile unit and not the skeleton unit in the main file. This means it uses the dwo abbreviation list and everything. Now, as far as I can tell, LLDB is kind of right here. The DWARF5 (in v4 split dwarf was a non-standard extension) spec says "A skeleton compilation unit has no children." (3.1.2 Skeleton Compilation Unit Entries). And indeed, gcc produces compilation units with no children. Clang, on the other hand, seems to be putting some DIEs into the main CU, if the compilation results in functions being inlined. And it seems clang has been doing this since at least 7.0 (I haven't tried older versions).

So it seems we have two problems here:

1. We need to figure out whether there's a bug in clang and fix it. + @dblaikie for help with that

Can you commit this yourself - or would you like me to do it for you?

Seems good to me - thanks a bunch (:

Might be easier as a few patches - renaming the existing option, adding the new one, then removing the single split dwarf flag handling in favor of implying that by the absence of an output file name. (if I'm reading what this patch does)

Generally looking pretty good to me.

Is this the change we were discussing the other week semi-related to preserved enums? It sounded to me like the conclusion was somewhat to go ahead without adding all these things to the constant list and see how it goes? Is this case different from the enum case in some way?

Any chance of a test case for this? (if you replace the null check with an assert for non-null, that might help identify a test case that hits this code?