- User Since
- May 9 2013, 11:10 AM (228 w, 1 d)
Thu, Sep 21
Well then! LGTM.
This came out of our sample-PGO work, wasn't sure it would be kosher for me to approve a patch from the guy in the next cube.
It's completely in line with the other similar patches that Andrea and Dehao have been doing. So if Craig is happy, I'm happy.
Wed, Sep 20
Fri, Sep 15
A few minor commentary points; nothing huge. I've been looking at this mainly with the thought of having to do something similar for DWARF 5 eventually, but it seems likely there won't be a lot to leverage as CodeView and DWARF appear to associate the hashes with files pretty differently. Oh well.
Thu, Sep 14
My impression is that .eh_frame and .debug_frame are interchangeable and in fact you get only one or the other in any given compilation. So, if you ask for .debug_frame and the object has .eh_frame, I think we should still dump it; and vice versa.
Wed, Sep 13
Tue, Sep 12
UI is tricky. I think I lean in the direction that if the section doesn't exist, don't print anything, even the "contents:" line.
This is certainly how I want --all to behave, and obviously it's easiest and self-consistent if that's how the tool always behaves. So for example --debug-types when there is no .debug_types section should print nothing.
I could see an argument that if I explicitly asked for a particular section and it doesn't exist, I should get the "contents:" line with nothing after it. But that's really not how I want --all to behave.
If you use cmake to generate Visual Studio projects on Windows, do they use lit to run the tests? If so, do they run with this patch correctly? I have no opinion one way or the other but it's an environment nobody seems to have thought of before (including me).
Mon, Sep 11
I would kind of like one option to dump whichever of .debug_info or .debug_info.dwo (etc) there is, so I don't have to know up front which section to be asking for. Would also be more user-friendly to people who kind of know DWARF but not at that level of detail.
I see that findPrologueEndLoc() is also checking the FrameSetup flag, so at least this makes the handling consistent, and relying on FrameSetup actually seems cleaner than relying on source locations. If you have time to investigate why the source locations seem wrong, that would be great.
LGTM after you fix the comment Adrian pointed out.
Fri, Sep 8
I think the argument (and the test) would be stronger if it was based on the instruction sequence rather than offsets. For example if you generate asm and then show prologue_end is immediately before the call to get_arg.
Also we generally like to trim the IR source by removing unnecessary attribute sets.
r312803. Forgot to put the tag in the commit message....
Thu, Sep 7
The new file goes in the debuginfo-tests directory. It's a separate project so that's probably not obvious from the diff.
Seriously simplified test (derived from DebugInfo/X86/discriminator2.ll, as suggested by dblaikie).
Tue, Sep 5
The patch seems trivial enough but I do not have a good idea of the subtle differences in the various environments.
I have subscribed a couple of people who probably can tell whether this change makes sense.
An MIR test that set up the situation more directly might be better, although I am not super confident in my ability to hand-reduce an MIR input.
Thu, Aug 31
I now think the tactic to pursue is making sure the containing DICompositeType(s) for the method have complete descriptions, and RAUW the temporary nodes, prior to calling CloneFunction. Actually wading around in the MDNodes, Types, and Decls to accomplish that result promises to be "interesting" and "educational" (advice/suggestions welcome).
Tue, Aug 29
Mon, Aug 28
Dumping the whole module, the temporary DICompositeType node for CBdVfsImpl is used both as the scope node for the DISubprogram, and also as the baseType of a DIDerivedType which is a pointer type in the type list for the subprogram.
Trying to understand the broader context here, I looked back through the list of revisions mentioned in PR33930 to see if that helped.
Fri, Aug 25
Sorry it took so long to wrap my head around this, but it has been about 20 years since I've had to accommodate an intentional ABI breakage, and that's actually what you want to do. Remembering that case, I have a model for this one, and I understand what you are doing now.
I'm happy, but Adrian should have a chance to comment too.
The DWARF2 restriction looks like it is related to non-contiguous code ranges, which were added in DWARF 3. So the warning should apply to code sections only. In that respect, functionally I think it's okay.
The diff needs to be refreshed as the surrounding code has changed in the interim, and it still needs a test.
There are two copies of DWARFExpression.h?
I would prefer a diagnostic if PS4 && >3.2. Whether you map "latest" to 3.2 or diagnose that also, up to you, I'm okay either way.
Thu, Aug 24
Locally we have a couple different tactics for dealing with changes that we can't support. A more coherent approach would be great.
For example we defined a new TargetCXXABI::Kind value that is mostly GenericItaniumABI except where it isn't.
I personally did not do most of the various ABI-related tweaks so I don't claim to have a good handle on them, and we have been slow to get these things upstream; but I'd love to make that happen.
Aug 22 2017
I mentioned this in the PR but I should have restated it here, sorry...
Wolfgang authored this patch. He is away for a month so I volunteered to post this, in case it was okay for resolving the PR as the crash is present in the 5.0 branch.
I do know Wolfgang was not really happy with it, and your questions might well be part of why that is.
The patch has been in our local version of 5.0 for a couple of weeks without obvious problems, but that is not proof that it is always okay.
We like to have the original source available for tests like this, ideally with commands to regenerate it. That practice helps people to understand the test and makes it easier to accommodate any syntactic changes that might come along (although that's more the case with IR tests than assembler tests).
Aug 8 2017
FTR, from a PS4 perspective this is all good, but we'd like somebody from outside our team to take a look.
Aug 2 2017
I'm happy with the test changes and the code looks straightforward enough. LGTM.
Aug 1 2017
Jul 27 2017
Jul 24 2017
Jul 20 2017
I am pretty sure that any verification you want to do on .debug_abbrev would be equally applicable to .debug_abbrev.dwo; I suspect we could also verify that .debug_abbrev.dwo did not use certain forms, e.g. DW_FORM_addr, but that would be a bonus.
Jul 19 2017
Wouldn't you also want to verify .debug_abbrev.dwo?
Jul 18 2017
Sweet. I don't have the chops to understand the Objective-C test but otherwise LGTM.
Jul 14 2017
Jul 13 2017
Jul 11 2017
Refresh to current TOT, and ping. Funny what you can find in a year-old to-do list....
Jul 10 2017
Jul 6 2017
Jun 30 2017
Jun 29 2017
Thanks for taking time with this!
Jun 28 2017
Removed RelocAddrMap members from DWARFDebugLine and DWARFDebugLoc.
DWARFDataExtractor ctors accept either a DWARFSection (can have relocations) or StringRef (no relocations).
Replace more DataExtractor parameters with DWARFDataExtractor.
Replace free function getRelocatedValue() with equivalent methods on DWARFDataExtractor.
DWARFUnit's LineSection changed from StringRef to DWARFSection, to make it harder to mess up the relocations to use with it.
Jun 27 2017
According to the top-level CODE_OWNERS.TXT the overall LLD code owner is @ruiu so please add him as a reviewer.
Also all LLD patches should have llvm-commits as a subscriber.
Note that when you add the list as a subscriber after-the-fact, you should repeat the original patch description as a comment, otherwise it won't appear on the mailing list.
Jun 26 2017
Jun 23 2017
I am mildly curious how much noise this adds to a build log.
As a follow-up, I was thinking that the DwarfFormat enum could be given a 1-byte base type (given that it has only two values), which would make DWARFFormParams only 4 bytes, which would make it reasonable to pass around by value instead of by reference.
See D34570, which packages the version/address-size/format and puts the query methods in one place. DWARFUnit and DWARFDebugLine use the new class as a data member, as suggested by @dblaikie. I'll update this patch again after that one's resolved.
Jun 19 2017
I'm not super excited about the hybrid form, but always unpacking a Unit inside the DWARFFormValue methods felt kind of expensive.
Jun 13 2017
A couple of minor comments inline. I'm okay with this, assuming Adrian is.
Please subscribe 'llvm-commits' when you create a review, so the whole community can see the proposed patch.
(If you add llvm-commits later, we recommend that you paste the original description into a comment, because otherwise the mailing list never sees the original description.)
Jun 12 2017
Provide DWARFDebugLine parsers with the DWARFContext, not just a relocation map.
Add a second overload of DWARFFormValue::extractValue() for use by the line-table parser; both overloads call a common private implementation.
This allows the v5 line-table header to use DW_FORM_strp.