- User Since
- Oct 8 2012, 9:19 AM (502 w, 4 d)
Wed, May 25
Right, so this does only reproduce with Split DWARF + Split DWARF inlining + ThinLTO, I think. The CUs for the imported units Split DWARF inlining are being designated as split units, but then split units aren't actually emitted (because of the single-unit limitation of Split DWARF) and so the pubnames are emitted for DIEs that were never layed out/emitted, by the looks of it, so they get an offset of zero, which looks like/creates a truncated pubnames contribution.
Abandoning this since the discussion ended up pointing towards a preference for a distinct noipa attribute. Then that stalled out in discussions about whether that could be implemented via isInterposable or would need a separate proprety and then revisiting/restesting every check for interposability to instead test "interposable or noipa", basically... which made me a bit exhausted to think about visiting/changing/retesting all those cases and so I haven't picked this up again. It's over in D101011 if anyone wants to weigh in there/pick it up/poke it with a stick, etc.
Sorry, with all the layers in the previous messages I'm not sure what the summary is. Sounds like the summary is "this is OK to continue work in the direction of supporting [[clang::warn_unused_result]] (only that spelling) for typedefs in C and C++ (for compatibility with C/so that C headers, like LLVM C's APIs can still be parsed as C++), while considering some of the complications raised in the WG21 discussion"?
Separate out clang:warn_unused_result so typedef support can be added to only that spelling.
Sounds good - maybe add some static_asserts in the unit test to validate that these operations are appropriately deleted? & could you make the comment more definitive - "Delete these operations as the defaults would be shallow/cause incorrect shared state. Implement these if/when needed/worthwhile to do so." or something like that.
Looks good - thanks for your patience & considering alternatives/helping me understand what was going on here.
Tue, May 24
@rsmith I had a few responses to your last round of review here - could you have a look through them/see if this is the right way to go, or if more refactoring would be suitable?
It doesn't make sense to require a stable hash algorithm for an internal cache file.
If you want to add any of the history, looks like this is my first email proposing the direction: https://lists.llvm.org/pipermail/llvm-dev/2015-February/081822.html
Looking at this further, if we were going to add a copy, it'd be good for it to be more efficient & probably suitable to have an efficient move operation too - but that's all a bit complicated/bigger project than I think we should dive into right now - so let's go with the D125611 solution for now.
Looking at D125899 in more detail I'm OK with deciding that adding good quality copy/move semantics to IntervalMap is a bigger project than I'm up for right now & so let's go with this direction instead (sorry for the back and forth - thanks for understanding).
Mon, May 23
Any chance of a template to avoid duplicating this code between case sensitive and case insensitive - looks like a long enough function with room for bugs/fixes/changes that it'd be worth avoiding duplication?
Sat, May 21
With ThinLTO enabled we can have a function in a CU that doesn't have split dwarf inlining enabled that gets inlined into CU that has it enabled.
When we try to lookup Abstract Scope Die for the inlined scope in constructInlinedScopeDIE, it's not present and we assert.
You mention performance concerns - even in an optimized build? What sort of performance data do you have? Would be good to have that documented here in the review/in the commit message to justify the use of these more error-prone APIs.
Might be worth a note in the Release Notes given that it's at least caused issues with one DWARF consumer.
Test coverage? (possibly just modifying much of the existing testing to occur in a constexpr context, if possible)
(any chance we can avoid duplicating this, and instead refactor the lld one into LLVM and have lld use that one instead of using its own local copy?)
Thu, May 19
I think the right solution to this is not to have zero sized symbols at all (they break C++ function pointer uniqueness, make symbolising "weird"/ambiguous, break aranges here, and if they are functions and those functions are called they result in some pretty arbitrary badness as execution falls off - and we should probably fix that in general so you can't just fall off the end of a function, the code size wins aren't enough to justify it I believe/think/would be good to have some data to back that up)
Wed, May 18
Tue, May 17
Looks OK otherwise
Mon, May 16
Sure, sounds OK.
Some more details on the specifics in the patch description would be good - I'm still not 100% clear on the reproduction case to understand what we're working around, exactly. Do you have a godbolt reproduction/small example, by chance?
Seems good - if folks find the growth too much, or it creates weird perf issues otherwise, we can discuss that if/when it comes up.
Fri, May 13
sounds ok, thanks!
Thu, May 12
(ah, might not be causing existing breakage because we're building the libc++ as a module, so stdatomic before/after doesn't interfere with the module, perhaps... )
Looks about right at a glance - thanks!
Can't say I'm following the details - but does seem like the code is correct, yeah? Though I'm open to workarounds, within reason. Happy to help throw around ideas if someone can show me a small/workable example on godbolt, for instance.
Wed, May 11
Tue, May 10
Mon, May 9
Thu, May 5
Yeah, mutating the DICompileUnit at this point seems a bad idea - it could end up inconsistent between things that happened before the mutation V after, etc.
Sure, if that works.
Doesn't look like all the cases are tested? (eg: I searched for basic_iostream and I only see the source change but no test change) - I realize some of these may be previously missing test coverage, but would you mind checking/adding the missing coverage?
Tue, May 3
Tried this patch on some internal targets and metrics and it seems actually quite reasonable.
Mon, May 2
Yeah, I'd go with the fix at memcpy in D122604 instead.
Would an explicit naming be more suitable than a region start/end? (I'd have considered this feedback for D116503 too, but didn't catch that one in review) The region based thing makes non-positional arguments weirdly positional (not that these are the first instances of that in compiler/linker tools, I don't think) - and I guess the override behavior is already positional to some degree.