looks good - thanks!

Test case?

Looks good - thanks!

Should the address size be required/used for parsing the debug_loc.dwo
section? I would think there shouldn't be any addresses, or otherwise
address-size-dependent there?

Seems good to me, then.

Looks good - thanks a bunch!

Sounds good - thanks!

Have you tested this with binutils dwp tool? Given the overlap between linking and dwp (binutils dwp uses parts of the gold linker implementation I think) - I could imagine marking these as ignore might even end up applying to the dwp tool & breaking that process.

Do you/what's your particular use case for this scenario? I guess this looks a bit like Apple's format (where debug info stays in the object file and isn't linked into the final binary), but don't expect they'd be moving to this any time soon.

Thanks for the fix! (optionally, could put some static_asserts in the DenseSet unit test to validate that these changes have been made (is_const<...> etc)

Looks good - thanks!

Fixed in r342510 with the solution I mentioned up-thread.

In D52191#1238648, @srhines wrote:

In D52191#1238628, @dblaikie wrote:

Sure, looks good. Though my other/vague concern is why does this case error about fomit-frame-pointer having no effect, but other things (like using -fomit-frame-pointer on a target that requires frame pointers) that ignore -fomit-frame-pointer don't? Weird. But it probably makes sense somehow.

I think the original issue is that one should not explicitly say "omit frame pointers" and "instrument for profiling with mcount". It's possible that there should be other errors for using "omit frame pointers" on targets that require them, but that should be checked independently elsewhere. Do we have many (any) of these kinds of targets? I'm going to submit this patch, but am happy to add a diagnostic later on for the other case, if you think that is worthwhile.

In D52241#1238637, @ruiu wrote:

What I found is lld consumes a lot of memory when we feed object files containing .zdebug_gnu_pub{names,types} sections because it uncompresses them in memory. I don't know how realistic that scenario is, but that's at least I tried.

Currently, if no --gdb-index is given, lld just concatenates .{,z}debug_gnu_pub{names,types} by section name because that's the default behavior of the linker. Can external tools consume such concatenated sections?

Sure, looks good. Though my other/vague concern is why does this case error about fomit-frame-pointer having no effect, but other things (like using -fomit-frame-pointer on a target that requires frame pointers) that ignore -fomit-frame-pointer don't? Weird. But it probably makes sense somehow.

It's possible another tool (gdb itself, for example) could read these sections and construct an index themselves - just later on, but still more efficient than parsing all the DWARF, etc. So I'm not sure if this should be the default or only behavior (not saying it shouldn't be - I'm genuinely unsure).

Sounds good - thanks!

Fair question - for Google, stream uncompressing to disk wouldn't help
matters - disk is a ramfs, so it's the same as uncompressing the whole
thing to a buffer in memory, which hurts a bit (due to memory limits).

*nod* It might be possible to (maybe too low-level - not sure if zlib
exposes this, or if the format even allows it to be efficiently answered)
retrieve the size of a compressed section without decompressing it (at
least in zlib-gnu fromat, I think the uncompressed size is written before
the compressed data, so easy there) - that way maybe more things wouldn't
need to care about whether it was compressed or not - it could be
decompressed lazily (& then the other half would be to deallocate promptly,
as soon as those bytes were finished with/written out to the output and no
longer needed).

Out of curiosity - would it be possible/useful to avoid keeping all the
pubnames sections uncompressed at the same time? Would they be able to be
processed one at a time (uncompress 1, process it, delete it, uncompress
the second, process it, delete it, etc)?

Looks good!

Looks good!

In D51908#1232920, @vext01 wrote:

It seems to me that the correct thing to do with unreachable labels is to emit a label with no low_pc attribute and fix gdb to accept such labels. Instead of crashing, it should print a message like "label was optimised out".

In D51908#1231469, @HsiangKai wrote:

If it has no DW_AT_low_pc in label debug info, GDB seems to assume it is 0x0.

(gdb) info scope foo
Scope for foo:
Symbol done is a label at address 0x400551, length 8.
Symbol unreach is a label at address 0x0, length 8.

llvm-dwarfdump results:

0x00000056:     DW_TAG_label
                  DW_AT_abstract_origin (0x00000075 "done")

Technically, I would think it might be best to produce the debug info for the label in the appropriate scope, but without an address - so that users can see that the label is there, name lookup finds that label not some other label or object?

Just to clarify - the general philosophy (there are many cases of exceptions due to complications in various areas) is that clang changes should have clang tests (source code to IR), LLVM changes should have LLVM tests (IR to assembly or object code+dwarf dump) & if debuginfo-tests are optional/as-desired.

Beats me why I disabled those RUN lines in r264350 - maybe it was just while I was testing & accidentally got checked in. Thanks for that!

In D51554#1224049, @echristo wrote:

The change in name here from "line tables" to "directives only" feels a bit confusing. "Limited" seems to be a bit more clear, or even remaining line tables only. Can you explain where you were going with this particular set of changes in a bit more detail please?

Looks good - thanks

Looks good to me & the answers to others various questions seem well addressed.

More or less agreed with @chandlerc (thanks for covering that all in detail) - though I'm marginally less concerned about the precedent setting in LLVM (small enough project we can rely a bit more on code review to discuss these things - and less likely to end up baked in so heavily that refactoring designs that are later discovered to be problematic to remove the use of this sort of thing).

Yep, I'll ask for the explanation - at least at Google there's been some
pushback on adding such a general purpose construct to the core library on
the premise that there's often/usually a better design. Equally I've
certainly seen some places it felt kind of right, for sure.

Not sure that every test for line-tables-only needs to also test line-directives-only, but not a huge deal either way. (only the cases where there's actually a different codepath to test/new code to validate would I bother testing - rather than duplicating all the tests)

In D48175#1219832, @scott.linder wrote:

The use I have in mind is actually an external component which depends on LLVM. We currently implement reading MessagePack metadata there, but when we land the changes to generate it in LLVM it would be nice to avoid duplicating the code there.

The rationale for a shared_ptr tree is that our API hands out opaque handles to sub-trees in the metadata which have their own lifetime independent of the root. With shared_ptr this just naturally falls out, but with unique_ptr it requires some book-keeping.

I do not see a scenario where the shared_ptr will be needed in LLVM, so if you prefer I am happy to update the patch to use unique_ptr.

In D48175#1216609, @scott.linder wrote:

I've updated the patch to just support shared_ptr; I think the more generic option is right for now.

Sure - seems plausible

Could you still come up with a practical test case for this? (doesn't
necessarily need to be checked in, to be honest - but something to validate
that this use case/support is needed & not working around a bug (& if it is
working around a bug, that we know about it so we can remove the workaround
eventually & not leave it in as folklore and cruft))

But if LLDB has different performance characteristics, or the default should be different for other reasons - I'm fine with that. I think I left it on for Apple so as not to mess with their stuff because of the MachO/dsym sort of thing that's a bit different from the environments I'm looking at.

These are the numbers from my llvm-dev email in June:

setting a breakpoint on a non-existing function without the use of
accelerator tables:
real 0m5.554s
user 0m43.764s
sys 0m6.748s

setting a breakpoint on a non-existing function with accelerator tables:
real 0m3.517s
user 0m3.136s
sys 0m0.376s

This is an extreme case,

In rL340583#284967, @bjope wrote:

We've got MIR tests (out-of-tree target) that runs the branch-folder pass.

Input has metadata like this:

!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, isOptimized: false, runtimeVersion: 0, emissionKind: NoDebug)
!1 = !DIFile(filename: "foo.c", directory: "b/")
!2 = !{i32 2, !"Dwarf Version", i32 4}
!3 = !{i32 2, !"Debug Info Version", i32 3}
!4 = !DILocation(line: 2, column: 2, scope: !6)
!5 = !DILocation(line: 3, column: 3, scope: !6)
!6 = distinct !DILexicalBlock(scope: !7, file: !1, line: 1, column: 1)
!7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: true, unit: !0)

and we are merging locations for these bundles

bb.1
    BUNDLE implicit-def $sp, implicit $sp, implicit-def $spmin, implicit $spmin, implicit-def $ccreg, implicit $cuc, implicit-def $af0, implicit-def $a0_40, debug-location !4 {
      addsp_nimm7 10, 0, $noreg, 0, implicit-def $sp, implicit $sp, implicit-def $spmin, implicit $spmin, debug-location !5
      dead $a0_40 = negh_a16_a40 killed $a0h, 0, $noreg, 0, implicit-def $ccreg, implicit $cuc, implicit-def $af0, debug-location !4
    }

bb.2
    BUNDLE implicit-def $sp, implicit $sp, implicit-def $spmin, implicit $spmin, implicit-def $ccreg, implicit $cuc, implicit-def $af0, implicit-def $a0_40, debug-location !4 {
      addsp_nimm7 10, 0, $noreg, 0, implicit-def $sp, implicit $sp, implicit-def $spmin, implicit $spmin, debug-location !4
      dead $a0_40 = negh_a16_a40 killed $a0h, 0, $noreg, 0, implicit-def $ccreg, implicit $cuc, implicit-def $af0, debug-location !4
    }

As seen the addsp_nimm7 has different debug-location in the input (!4 vs !5).
In the past the result of getMergedLocation was a nullptr here (NoGeneratedLocation), and the debug-location was dropped. But with this patch we get a generated location back (which should be a line 0 loc).

The problem is that the generated debug-location is written like this debug-location <0x4af5768> in the MIR output from the pass:

BUNDLE implicit-def $sp, implicit $sp, implicit-def $spmin, implicit $spmin, implicit-def $ccreg, implicit $cuc, implicit-def $af0, implicit-def $a0_40, debug-location !4 {
  addsp_nimm7 10, 0, $noreg, 0, implicit-def $sp, implicit $sp, implicit-def $spmin, implicit $spmin, debug-location <0x4af5768>
  dead $a0_40 = negh_a16_a40 killed $a0h, 0, $noreg, 0, implicit-def $ccreg, implicit $cuc, implicit-def $af0, debug-location !4
}

So there is no new metadata reference for the debug-location.
Is guess this is a common problem for all backend passes that used to pass NoGeneratedLocation to getMergedLocation, so maybe there is some missing logic to handle that we get a new DILocation in those passes?

@dblaikie (or @aprantl who has made further patching on this patch), do you know what's missing here (let me know if you need a full test case)?

In D51208#1212320, @labath wrote:

As far as the strict intention of this test goes, the change is fine, as it is meant to check that the accelerator tables get used *when* they are generated. How do you achieve them being generated is not important.

However, I am not sure now under what circumstances will the accelerator tables be emitted in the first place. David, does this mean that we will now not emit .debug-names even if one specifies -glldb? Was that intentional?

In D44429#1210470, @kristina wrote:

Don't have commit access but this should be revised to update the modulemap.

module LLVM_BinaryFormat {
    requires cplusplus
    umbrella "BinaryFormat" module * { export * }
    textual header "BinaryFormat/Dwarf.def"
    // -snip-
    textual header "BinaryFormat/WasmRelocs.def"
    textual header "BinaryFormat/MsgPack.def"
}

Modular build passes after that.

In D44429#1210457, @kristina wrote:

Causes a regression with modular builds here:

[168/3291] Building CXX object lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/Dwarf.cpp.o
FAILED: lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/Dwarf.cpp.o
/usr/local/sdk/llvm-8.0.4078/bin/clang++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/BinaryFormat -I/SourceCache/llvm-trunk-8.0.4079/lib/BinaryFormat -Iinclude -I/SourceCache/llvm-trunk-8.0.4079/include -O3 -march=skylake -dwarf-4 -rtlib=compiler-rt -Wno-unused-command-line-argument -stdlib=libc++ -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -std=c++11 -fmodules -fmodules-cache-path=/o/org.llvm.caches/llvm-8.0/4079/module.cache -Xclang -fmodules-local-submodule-visibility -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -ffunction-sections -fdata-sections -flto=thin -O3    -UNDEBUG  -fno-exceptions -fno-rtti -MD -MT lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/Dwarf.cpp.o -MF lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/Dwarf.cpp.o.d -o lib/BinaryFormat/CMakeFiles/LLVMBinaryFormat.dir/Dwarf.cpp.o -c /SourceCache/llvm-trunk-8.0.4079/lib/BinaryFormat/Dwarf.cpp
While building module 'LLVM_BinaryFormat' imported from /SourceCache/llvm-trunk-8.0.4079/lib/BinaryFormat/Dwarf.cpp:14:
In file included from <module-includes>:6:
/SourceCache/llvm-trunk-8.0.4079/include/llvm/BinaryFormat/MsgPack.h:30:1: error: redundant #include of module 'LLVM_BinaryFormat.MsgPack' appears within namespace 'llvm::msgpack::FirstByte' [-Wmodules-import-nested-redundant]
#include "llvm/BinaryFormat/MsgPack.def"
^
/SourceCache/llvm-trunk-8.0.4079/include/llvm/BinaryFormat/MsgPack.h:28:1: note: namespace 'llvm::msgpack::FirstByte' begins here
namespace FirstByte {
^
/SourceCache/llvm-trunk-8.0.4079/include/llvm/BinaryFormat/MsgPack.h:42:1: error: redundant #include of module 'LLVM_BinaryFormat.MsgPack' appears within namespace 'llvm::msgpack::FixBits' [-Wmodules-import-nested-redundant]
#include "llvm/BinaryFormat/MsgPack.def"
^
/SourceCache/llvm-trunk-8.0.4079/include/llvm/BinaryFormat/MsgPack.h:40:1: note: namespace 'llvm::msgpack::FixBits' begins here
namespace FixBits {
^
/SourceCache/llvm-trunk-8.0.4079/include/llvm/BinaryFormat/MsgPack.h:55:1: error: redundant #include of module 'LLVM_BinaryFormat.MsgPack' appears within namespace 'llvm::msgpack::FixBitsMask' [-Wmodules-import-nested-redundant]
#include "llvm/BinaryFormat/MsgPack.def"

Looks good - thanks!

Looks good - thanks!

In D50945#1205337, @kristina wrote:

Given the context (class an file name itself) and documentation around the function, I don't think in this particular case it improves readability or maintainability, the lifetime of the HeaderMap is (IMHO) fairly obvious from the const qualifier and from the documentation of the function itself. I would say leave it as is.

In D50213#1202568, @aprantl wrote:

Sorry, my mistake. I saw that you added a new field in the IR assembler and sloppily assumed that there is also a new bitcode record for it, but there isn't, so the whole point is moot.

In D50213#1201350, @aprantl wrote:

Can you add a bitcode upgrade test?

I could - but I'm not sure it would test anything. The format has in some sense remained the same - this field is still zero/one before/after the change, but now new values are also valid.
Reckon it's still worth testing? What do you reckon that testing would cover?

It would be sufficient to find an existing .bc file with a DICompileUnit in it and add a CHECK: that tests that the new flag comes out as default(=0) (which probably means it won't be printed at all, but that's fine).

In D50213#1200917, @aprantl wrote:

Currently has no effect, but see above ^ - I reckon it probably should. Maybe we can generalize the name of the property? "nameTable: " {None, Default (v4 or v5 based on DWARF version), GNU (only makes sense for v4 though - ignored if you end up building v5? seems reasonable to me)}

Is there a test covering the DWARF5 + GNU CU flag combination?

ping

In D50476#1197511, @jfb wrote:

In D50476#1197364, @dblaikie wrote:

You could use D.count(V) to save the comparison with end, etc.

I don't understand what you're suggesting, could you clarify?

You could use D.count(V) to save the comparison with end, etc.

probably leave this a bit more to @aprantl and @JDevlieghere (figure they can speak more to the LLDB needs/tradeoffs/etc that this is targeting) - but there's a first pass.