So I can see where/how this fails now - the LLVM backend seems to require that if any file has embedded source, they all do. Would you be able to/would you mind adding a debug info verifier check for this? That'd help make this fail sooner. (@scott.linder - perhaps you'd be able to/interested in doing this - looks like you wrote the initial implementation?)

Agreed - looks like this went in untested in r175813 & has never been used.

adding @lhames just as FYI for some stumbling over the Error-as-out-parameter usage.

Thanks for this - though it looks like the test program hits an assertion failure (for me at least - before it gets to the interesting point.

Looks good - thanks!

Looks good - thanks!

Looks good - thanks! just a few things that could be cleaned up.

Looks good - thanks! Do you need me to commit this for you? (Or do you have commit rights & can commit it yourself)

Is this test case consistent with gold's behavior? (given this subprogram has no source locations here, for instance - does gold still consider such a simplified DIE to be a thing that should be indexed?)

Settled on renaming the flag for consistency with, say, -fdebug-types-section, to -fdebug-ranges-base-address (though 'debug' is sort of ambiguous (Clang can produce non-DWARF debug info) but this driver (as opposed to clang-cl) is intended for DWARF emission, not PDB emission - and the "debug-ranges" part of the flag names the literal debug_ranges section (like debug-types refers to the debug_types section)).

Settled on renaming the DICompileUnit attribute from debugBaseAddress to rangesBaseAddress (since 'debug' is redundant, and this is really specific to ranges base address specifiers).

Sounds good to me. Where possible it's great to quote the specs more than the source code - did you happen across any spec for gnu_pubnames themselves? Or only the gdb-index they're used to create (& that's good too - having consistency of naming between gnu_pubnames and gdb_index certainly helps)

Seems reasonable - the spec https://sourceware.org/gdb//onlinedocs/gdb/Index-Section-Format.html also describes these as "CU index+attributes" values.

(ah, I see I hadn't posted my first feedback earlier (written but unsubmitted) - oops, sorry about that)

Hey @rjmccall - I'm trying to remember, but can't get it quite clear in my head. I seem to recall some discussion about trivial_abi not implying/being strong enough for all the cases that trivial_relocatable sounds like it covers? Do you happen to remember the distinction there (the summary in this patch ("the warranting attribute [[trivially_relocatable]], which is similar in spirit to [[trivial_abi]], in that it lets the programmer communicate back to the compiler that a certain user-defined type should be assumed to have this property even though it would not naturally have the property all else being equal.") doesn't sound like what I remember - but my memory is hazy)?

Could you fix the modulemap (might amount to reverting the change Eric made in r342827? or maybe it's more involved than that) & validate that the modules build is working with this change (probably undo Eric's change, validate that you see the breakage that Eric was trying to fix, then apply your change & see if it clears up as well)?

In D54166#1295730, @riccibruno wrote:

@dblaikie Thanks for looking at this patch !

I have a set of patches shrinking the other statements/expressions.
Can I add you to review some of these too ? Most of them consists of just moving
some data. I added rsmith but I think that he is busy with the standard.

Seems pretty reasonable to me!

Looks like this could use an LLVM CodeGen test too (similar to, or extending/renaming test/DebugInfo/Generic/mainsubprogram.ll)

Ah, OK - so it's not so much a problem of the file being read containing interesting 64 bit values, but the way the host architecture works that exposes this bug. Makes sense - thanks for walking me through it!

Sounds good to me - I'd usually put spaces inside the start/end of the comment (& maybe between the comment & the constant) but that's just me - I'm not fussed about which way it's documented (although - best it's documented whatever way clang-format formats it, so as not to be confusing)

Looks like the original functionality was added here in r109651 without any test case... so it's hard to say that because your change causes no regression that it's correct, because there's a lack of test coverage (no test tests for this message about "file number already allocated").

In D54424#1295850, @atanasyan wrote:

In D54424#1295814, @dblaikie wrote:

Any chance of a test case?

I hope the following test case starts to pass on mips buildbot:
http://lab.llvm.org:8011/builders/llvm-mips-linux/builds/3739/steps/test-llvm/logs/FAIL%3A%20LLVM%3A%3Adebug_addr.ll

In D54384#1295839, @rupprecht wrote:

cc'ing blaike as a local debug expert -- I'm not familiar with systems using --build-id

@mcgrathr 's comments are probably more relevant than mine - especially those that, by the sounds of it, remove the need for adding a new iterator (but I only found that after I'd already written some of that - and figured the comments might be useful general feedback/'learnings' anyway :) )

In D54336#1294282, @chandlerc wrote:

FWIW, this looks really good to me. I'd be fine with this going in as-is and any further improvements suggested by David as follow-ups. Feel free to submit with comments addressed.

Any chance of a test case?

(oh, also, looks like this is intended as a fix for PR39617? Good to mention that in reviews/commits - also, there's no need to file a bug before sending a patch. You can skip the bug & send the patch instead if you like!

Could you add a test case? (probably a static_assert in unittests/ADT/iterator.cpp or something like that)

OK - thanks for that.

I think this would fail at least in the following example:

Looks good to me! Seems to come out naturally/unsurprisingly/unlikely to be weird by DwarfFile::emitUnit - whether it's TUs, CUs, in different sections, the same section, interleaved or separate.

Looks great!

In D54217#1292327, @dblaikie wrote:

In D54217#1292323, @vsk wrote:

Why not check in a .o? The numbers won't change, so they can meaningfully be compared.

An object file or assembly seems fine, yeah.

In D54217#1292323, @vsk wrote:

Why not check in a .o? The numbers won't change, so they can meaningfully be compared.

In D54217#1291870, @dblaikie wrote:

In D54217#1291854, @clayborg wrote:

In D54217#1291834, @dblaikie wrote:

In D54217#1291820, @clayborg wrote:

yeah, an easy code search indeed kicked up the tests. I will add one. Sorry about that

No worries - still curious where you were looking for the tests initially, perhaps we can do something to make them more discoverable?

The only thing I could think of that could make it easier is to make a symlink from the tools/llvm-dwarfdump/test which points to ../../test/tools/llvm-dwarfdump

Ah, fair enough - yeah, might just be a hurdle we can live with, then. The tests mostly match the directory hierarchy - but yeah, there are a few quirks.

(another way I find tests: Break something nearby/related (eg: remove one of the existing statistics) & see what tests fail)

In D54217#1291828, @clayborg wrote:

Looks like I can piggy back on llvm/test/tools/llvm-dwarfdump/X86/statistics.ll

Does anyone know how to run only this test from command line?

from the build directory I usually run "./bin/llvm-lit -v test/tools/llvm-dwarfdump/X86/statistics.ll" for example (yeah, even though that relative path doesn't exist in the build directory, lit knows to also try resolving it from the source dir, etc)

Thanks! Will be an easy tests to add. One other FileCheck question. Seems like I would want to check that "total function size" and "total inlined function size" exist.
Can I rely on the exact byte sizes from this .ll file on all systems?

Only if the test has a specific triple. Which, by the looks of it, this test could benefit from. @aprantl - would that make sense? This test is only run when the X86 target is enabled anyway, so adding a specific triple wouldn't make it less portable - and would allow for specific statistic values to be tested, which seems like it'd be important for test coverage here (as it stands, byte count parts of this test look like it would still pass even if the statistics were miscomputed/all zero, etc).

In D54242#1291956, @JDevlieghere wrote:

Is this going to be used for anything other than ranges? If not I'd slightly prefer rangesBaseAddress over debugBaseAddress.

In D54217#1291854, @clayborg wrote:

In D54217#1291834, @dblaikie wrote:

In D54217#1291820, @clayborg wrote:

yeah, an easy code search indeed kicked up the tests. I will add one. Sorry about that

No worries - still curious where you were looking for the tests initially, perhaps we can do something to make them more discoverable?

The only thing I could think of that could make it easier is to make a symlink from the tools/llvm-dwarfdump/test which points to ../../test/tools/llvm-dwarfdump

In D54217#1291820, @clayborg wrote:

yeah, an easy code search indeed kicked up the tests. I will add one. Sorry about that

In D54217#1291793, @clayborg wrote:

In D54217#1291170, @vsk wrote:

Looks reasonable to me, but it'd help to check in a .o for a test.

There don't seem to be any tests for any of this. Are there existing tests for stuff that is only in llvm-dwarfdump? If so, please point me to where they are.

Remove unnecessary attributes and add test input description

While I'm not 100% sure about the actual fix - I'm confident enough that @rsmith can provide any further clarification in post-commit.

Thanks! - though on reflection I'm going to invoke @echristo again about the naming. It's unfortunately a bit backwards that the pre-standard flag is -gsplit-dwarf and what we're proposing as the standard flag is -fdwarf-fission, when the DWARF standard doesn't use the word "Fission" at all (only "split DWARF").

In D53329#1270035, @yonghong-song wrote:

Sure. Let me provide a little bit more context and what I want to achieve:

. I have a tool, called bcc (https://github.com/iovisor/bcc) which uses clang CompilerInvocation interface and
  MCJIT to generates BPF code and load into kernel
. Several files (the main.c and a few others headers) are clang memory mapped.
. The particular fix here is related to https://reviews.llvm.org/D53261, getting source code into BTF, but
  before that, based on that particular implementation, the source code needs to be in IR.
  What I found is that for the memory mapped /virtual/main.c file, there is one DIFile entry in
  generated IR, the associated 'source' is empty, which actually caused a seg fault later.
 
  Not that this bug itself does not need https://reviews.llvm.org/D53261.

So without this fix, bcc tool will seg fault.
With this fix, bcc tool works properly and all DIFile entry has proper source codes embedded, which
if coupled with IR->BTF or Dwarf->BTF should generate correct BTF debug info.

Generally looks good to me - I'll leave it to you & Adrian to hash out any concerns he has.

In D53334#1273877, @whisperity wrote:

@dblaikie I have created a test, but unfortunately %clang_cpp in LIT invokes clang --driver-mode=cpp which is not the same as if clang++ is called. I'm trying to construct the following command-line

clang++ -fmodules-ts -fprebuilt-module-path=%t/mods --precompile -c file.cppm -o file.pcm

However, using %clang_cc1 I can't get it to accept --precompile as a valid argument, and using %clang_cpp I get an "unused argument" warning for --precompile and the output file is just a preprocessed output (like -E), which will, of course, cause errors, but not the errors I am wanting to test about.

Looks like Eric still had some questions here? Or at least hadn't commented
about whether the answers/changes addressed his concerns?