- User Since
- May 9 2013, 11:10 AM (280 w, 5 d)
Fri, Sep 21
Do we generate the .dwo file directly these days? If not, I can imagine wanting to avoid the overhead of the objcopy hack; as long as the linker is smart enough not to bother with the .debug_*.dwo sections this seems like a build-time win.
Wed, Sep 19
Tue, Sep 18
Mon, Sep 17
Fri, Sep 14
Thu, Sep 13
Wed, Sep 12
Can you preserved the declaration DIE (at 0x0000007c) while omitting the definition DIE (at 0x00000063)? Does that still cause a problem for GDB?
Tue, Sep 4
Tue, Aug 28
Mon, Aug 27
The indentation in the test looks a little odd. Please verify there are no hard tabs.
Aug 22 2018
Aug 10 2018
Aug 8 2018
Aug 7 2018
I'm abandoning this minor tweak as I am convinced that there is something more fundamentally wrong here.
llvm-dwarfdump doesn't call the right API sequence to show the bug; it requires a DWP, looking up a specific unit by hash via the DWP index, and then scanning all the units. I can envision how to do a unittest for that, but the unittest infrastructure doesn't currently support building a DWP. I can't take the time right now to address this, but I'll keep it on The List.
Aug 6 2018
Aug 3 2018
How does this interact with v5 .debug_names?
Two totally optional style nits. The new class distinction seems like an improvement.
Aug 2 2018
Aug 1 2018
Jul 31 2018
Using %t with suffixes instead of %T is the preferred idiom, glad you worked that out.
The more elaborate test depends on a set of Posix-y calls, but that's fine. If there are more environments that can't handle it, the bots will find them for you. :-)
LGTM with the fancier test, once you fix the missing RUN. (The RUN prefixes are how lit decides which lines are the test script.)
I think you forgot to subscribe llvm-commits to this review?
Rebase. Add to and improve commentary.
Rebase. Renamed 'parse' methods to start with 'addUnits' to reflect their cumulative nature.
Rebase. Update some commentary.
Jul 30 2018
Is it possible/reasonable for the test to call __llvm_profile_recursive_mkdir and then verify the mode of the created directory?
The test would have to be flagged as unsupported on Windows but that seems fine.
Jul 27 2018
As a partial and somewhat experimental feature, this should be controlled by a command-line option and off by default.
Even when it's complete it should probably still be under at least a tuning option, unless you can demonstrate the size cost is small.
Jul 26 2018
Although I am far from expert in how Clang manages declarations, AFAICT this does what @rsmith requested, so LGTM.
Jul 25 2018
Jul 24 2018
Jul 23 2018
Re. SmallVector versus std::vector, they are functionally similar but have different memory-allocation behaviors. SmallVector includes a vector of N elements (where N is the template parameter) so does no dynamic allocation until you have more than N elements; but it takes up that much more space as a member of a class. It's mainly intended for stack variables. Given that this particular option is used rarely, I would probably go with std::vector. The code change otherwise is pretty simple and looks fine.
cc llvm-commits. Please do this when first creating the review.
+ @clayborg who IIRC did the original generator
Also, please document the option in clang/docs/UsersManual.rst. It should have been added when the option was first added, but certainly with right-to-left behavior it needs a mention. Nearly all options work left-to-right, so even though it's the same as gcc, not everybody comes to clang from gcc.
Needs a test.
Jul 20 2018
Sorry, lost track of this one. One comment suggestion and LGTM.
If the plan is for this to be relatively temporary then LGTM.
It seems like the metadata could be reduced further. In particular, do you really need all those different types? The dbg.value needs only !11, I think you should be able to remove !12 through !18 (and references to them).
Is this because type units depend on COMDAT support? I had a vague idea that COFF also supports COMDAT.
Please rename the new test to simply 'codegenprepare-and-debug.ll` (the convention of a date on the test name is obsolete, we don't do that for new tests anymore).
Jul 19 2018
LGTM but I'll let @JDevlieghere have the last word. It looks like he still has one open question in DwarfDebug.cpp.
I agree the ordering test looks fragile. The only other option that comes to mind is trying to exercise the APIs directly from a unittest, but then you need enough scaffolding around it to capture at least the assembler output and verify it. Probably best to leave the test as it is.
@rsmith any further thoughts? We would really like to get this in before the LLVM 7.0 branch, currently scheduled for 1 August.
Jul 18 2018
A few typos.
A bunch of style comments. Maybe try clang-format-diff.
I wonder if we should experiment with making the line-table builder complain about a .loc that doesn't increment the address. That would tell us whether these zero-length cases show up anywhere else. Or, if we decide the assembler should handle it correctly, whether anything slips through.
Jul 17 2018
I will try to look at this tomorrow if @vsk doesn't. Passes are not really my thing but as it's based on the existing opt code it should not be too hard to review.
The repeated address is syntactically valid per the rules for the line-number program, but semantically undefined per the rationale in the non-normative description of the table that the line-number program is supposed to be encoding (i.e., a table indexed by instruction address).
This is slightly complicated by the fact that the debug_names tables are
incompatible with the DWARF v4 type units (they assume that the type
units are in the debug_info section), and unfortunately, right now we
generate DWARF v4-style type units even for -gdwarf-5.