- User Since
- Oct 8 2012, 9:19 AM (319 w, 2 d)
Sat, Nov 17
Fri, Nov 16
Thu, Nov 15
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.
Wed, Nov 14
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.
Tue, Nov 13
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)?
Mon, Nov 12
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)?
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").
@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 :) )
Any chance of a test case?
Sun, Nov 11
(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)
Fri, Nov 9
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.
Thu, Nov 8
Wed, Nov 7
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").
Tue, Nov 6
Generally looks good to me - I'll leave it to you & Adrian to hash out any concerns he has.
Mon, Nov 5
Fri, Nov 2
Looks like Eric still had some questions here? Or at least hadn't commented
about whether the answers/changes addressed his concerns?