- User Since
- Oct 8 2012, 9:19 AM (328 w, 2 d)
"mainly" used for std::sort might be true, but "only" used might be harder to prove - did you use any tooling to test that these inclusions weren't needed other than "this code still compiles" (which has the problem that the inclusion might be used, but also included indirectly from some other header - so removing it may still compile, but introduces an undesirable indirect dependency on an implementation detail of some other header)
I'm OK with this change from a "slightly reduces debug info size" but I hesitate to suggest that this isn't a thing your consumer should support & this "When an assembly file with multiple .loc directives with no interveaning instructions are assembled, GAS produces a DWARF line table that maps one instruction to multiple entries." sounds like a bit of a mischaracterization of how I'd read the line table entries in question - I think this line table doesn't "map one instruction to multiple entries" but instead has an empty entry that contains no instructions. Everything between one .loc and the next is at that first location - if there's nothing between them, then nothing is at that location. I suspect that's how other consumers are viewing this situation.
Seems good - thanks!
Thanks for filing the bug/reducing a test case - this change should include an automated test case that captures this issue (check for other tests that pass -gembed-source to see how this might be tested, possibly expanding the existing test case case or introducing a new one (test/CodeGen/debug-info-embed-source.c looks like the place to start)
Probably best to have someone else from AMDGPU land review this for your uses there - I'm just giving some high level at-a-glance stylistic coverage here.
Sun, Jan 13
Tue, Jan 8
No doubt @rsmith will have a more nuanced/accurate opinion on this than I do, but I would've thought the point of implicit modules is that they don't get moved around & it sounds like that's what this patch is suggesting/supporting, but maybe I haven't understood it fully?
Sun, Jan 6
Dec 24 2018
Dec 23 2018
Dec 22 2018
Dec 21 2018
Not sure I'm following this all in detail & at least for me (someone not super familiar with the details here) it might've been a bit easier to review in smaller pieces (breaking up the refactoring to generalize the components of the discriminator into smaller bits, etc) but all good. Thanks!
Dec 20 2018
Dec 19 2018
Might be worth having a summary of the goals here (rather than only by reference to the design discussion thread) for review and later as commit history, etc.
Dec 18 2018
Rename dumpArray to dumpArrayType
Implement & test behavior for subrange without any attributes (no low/high/count).
Pull out array printing into a helper function
Use language default lower bound to inform size calculation from upper_bound with no lower bound
Use [size] rendering when explicit lower bound matches language default
Dec 17 2018
Recommitted this, predicating everything on !useSectionsAsReferences, which means the existing sections_as_references passes unmodified & I added another separate test to validate the change. Thanks again @ABataev for that pointer/fix! (also validated it against the internal failure that caught this too)
But I'm still confused & would love some help understanding why/how some of these features are gated by the cl::opt -dwarf-sections-as-references or NVPTX - but other features are gated only on NVPTX? Should we remove -dwarf-sections-as-references and just rely on NVPTX for all these things? Or is there some other use of -dwarf-sections-as-references that isn't NVPTX?
Because we don't want to rely on a particular target type. Yes, currently it is used only for NVPTX. But later there might be some other targets with the same limitation and we can reuse the same option for all such targets.
Hey Alexey - thanks for taking a look!
(from the peanut gallery): Usually what I'd expect here is "reindexing will occur X milliseconds after a write, covering anything written up until that point" - so it doesn't reindex unnecessarily if the files are unmodified/sitting idle, and it doesn't thrash reindexing for multiple changes in rapid succession (such as hitting the "save all" button in some text editor - or having made a few other quick changes one after another maybe), but still provides reasonable up-to-date data for the user.
Sounds alright to me - thanks!
Dec 14 2018
Dec 12 2018
FWIW, I ended up adding test coverage for some of this in r348954 while improving the printing of arrays to include array dimensions - hand written assembly, so yeah, it's a bit less than ideal to maintain (mostly having to edit both the abbreviation and the definition - a solution to that would be handy (& having to lookup the numeric values of the enums (DW_AT/DW_TAGs))) but I think it's worth having.
Dec 10 2018
Dec 9 2018
Overall I'm not sure this construct/pattern improves readability, but not too fussed either way.
Dec 7 2018
Dec 5 2018
From what I'm seeing, I would imagine this may regress debug info quality a bit. In the narrow example given, the DWARF still ends up with a description of 'foo', but without any location/value - though this is the same as if 'foo' was in the same translation unit (looks like LLVM fails to track the removing of the global and replacing everything with a constant - I believe the debug info metadata supports describing such a situation, but it isn't being used here - with or without this new change). SO that's not a regression.
Dec 4 2018
Ah, I found a comment from the original review:
I'm unaware of any regression for debug info quality that occurs with this change, but I haven't done wide scale testing of it - does anyone have a deeper/practical/feature test that demonstrates the motivation for this part of the change in the first place?