- User Since
- Oct 8 2012, 9:19 AM (337 w, 11 h)
Fri, Mar 22
Assuming you've checked this is unused in other projects like lldb, lld, etc, sounds good to me - thanks!
Thu, Mar 21
Pleasue include mention of the bug (PR40276) in the commit message & clarify that while this is useful for some remote compilation models, it's not strictly necessary/the only way to do it (a remote compilation model that keeps relative paths and uses compilation-dir instead can work without this attribute)
Wed, Mar 20
This seems like it might be worth a broader discussion about these sort of cases - probably llvm-dev, maybe with some DWARF folks (though the usual LLVM debug info cabal (myself, @aprantl, @probinson, @JDevlieghere, and @echristo) is probably sufficient to get a rough idea of what might be a good general approach).
Tue, Mar 19
(also the title of this patch makes it sound like this is a patch to llvm-symbolizer (which I Suspect it should be - if gold/binutils ld produce similar output to lld's current (pre-patch) output, then llvm-symbolizer should be adjusted to cope with that, even if lld is changed))
What do other linkers (gold and gnu binutils, for instance) do in this case?
Fri, Mar 15
Does this need to be line zero, or could it be no location (the absence of a dbgloc)?
Thu, Mar 14
Thanks a bunch - looks good to me.
Hmm, trying to stare at this function it's confusing me a fair bit. I'd expect this to be more obvious/legible than it is, but it's possible I'm just misunderstanding.
Would it be simpler/better to revert all the FlagTrivial work? & use the FlagNonTrivial+composite type to imply trivial? (since FlagnonTrivial has been in-tree longer)
Wed, Mar 13
Tue, Mar 12
Great - thanks for your patience!
Mon, Mar 11
Looks good - thanks!
Sat, Mar 9
Looks good to me, thanks
Fri, Mar 8
Thu, Mar 7
Seems reasonable to me (but feel free to wait for someone with more context/domain awareness)
Wed, Mar 6
Might be worth a separate/targeted test with some of the interesting edge cases? (like empty range at the end of the sequence, etc)
Mind adding at least one example with neither triviality flag? (I guess a type that's not a struct or class? A union maybe?)
The code change touches two places, but the test only seems to test one branch instruction - is one of the two code changes untested?
Tue, Mar 5
Mon, Mar 4
The patch title doesn't seem related (or I don't understand how it's related) to the patch, and the patch description doesn't seem to detail how it relates to the code change.
I guess maybe we have unit testsn for some but not all of these wrappers - might be worth checking/sampling to see what the prevailing trend is (I tend to be over-enthusiastic about testing, but I realize these simple wrappers are pretty uneventful)
Fri, Mar 1
(food for thought, not a "this must be answered now" sort of thing, but "Generic atom-graph data structure and algorithms " does sound a bit like the original LLD design, which didn't turn out to be a great fit (at least the way it ended up) for ELF, at least - any ideas on what might be done differently to avoid the same situation here when adding ELF support down the line? (my naive perspective is that the mismatch was around slicing up ELF sections into MachO-like atoms on symbol boundaries was the mismatch - rather than adding support for an atom with multiple symbols (that may not be at the start of the atom) and then having a one atom to one ELF section mapping))
Looks great - thanks!
Thu, Feb 28
Would it be possible/practical/reasonable to split getOrCreateTypeDIE at 643, before the createAndAddDIE? And at that point do the switch to the appropriate CU (basically as soon as we get the context DIE and can determine which unit is the appropriate one to build the type DIE in)
How about we keep the discussion on the original patch (D57986)? I think we explored the problem space a fair bit there & I'll advocate for the same thing here as I did there: Sorting before emission is likely more efficient than keeping a continuously sorted container, if there's a separate "build" and "iterate" phase you can sort between. At least that's my understanding. std::map and MapVector would both grow memory usage, probably not prohibitively, but a fair bit.
Wed, Feb 27
Tue, Feb 26
I'll test a couple of related cases (implicit special members and nested classes) here in a moment & see if they have any related bugs.
Looks good - thanks!
Here's a bit of a smaller test that might make it clearer (could add some comments to help explain why each part is present):
Mon, Feb 25
Would it be simpler/effective to add "-fuse-ld=gold -Wl,--gdb-index" to your linker flags to enable both these things, rather than having specific CMake support for either/both?
Hmm, I don't seem to have had the troubles you've had - I use CMAKE_EXE_LINKER_FLAGS_DEBUG:STRING=-Wl,--gdb-index and... ah, I don't use (or even have in my CMakeCache.txt any mention of) LLVM_USE_LINKER. Not actually sure where/how my build is choosing to use gold. Perhaps it's just my system default once it's installed.
Feb 22 2019
I think the point where we lookup the other CU and cross over to it should probably be earlier - imagine if all attributes could potentially be CU-local (eg: we could produce a separate string_offsets section for each CU and so have separate strx indexes for each CU) & we should probably have a solution that would be correct in that situation. Does that make sense?
Feb 21 2019
Feb 20 2019
Looks good - thanks!
Feb 19 2019
I'm a touch confused by the whole discussion here - so I'll write some things & perhaps folks can correct my understanding where necessary.
Feb 18 2019
There should be an rvalue overload of std::vector<T>::push_back - is there not? ( https://en.cppreference.com/w/cpp/container/vector/push_back )
Feb 14 2019
Feb 13 2019
How does this relate to D57940?
This has some relatively extensive changes to some of the llvm tools (like sancov, for instance) that aren't tested - perhaps it'd be better to leave those tools usage unmodified (adding the undef section value as a fix to the API change, but nothing more) with fixits and/or followup patches that implement the improved functionality possible with the new API, along with tests?