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.

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.

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.

In D37756#868977, @zturner wrote:

In D37756#868778, @probinson wrote:

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).

I just tested running check-llvm from inside Visual Studio using a Visual Studio generated CMake project and it worked fine. I didn't run the rest of them, since if one works they all should.

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).

In D37714#867262, @aprantl wrote:

What about supporting both options, but --debug-info also implies --debug-info-dwo?

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.

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.

In D37604#864187, @aprantl wrote:

This seems reasonable to me, thanks!
When you commit this, could you please double-check that the tests are still running on the green dragon builders? I'll also keep an eye on them.

r312803. Forgot to put the tag in the commit message....

The new file goes in the debuginfo-tests directory. It's a separate project so that's probably not obvious from the diff.

In D37364#863979, @dblaikie wrote:

Seems OK to me

For a minor perk - would it be possible to test the line table length? Since the benefit (smaller line table) of this change isn't readily apparent in the high level line table printing that dwarfdump does. I know it'd make the test a bit more brittle.

Seriously simplified test (derived from DebugInfo/X86/discriminator2.ll, as suggested by dblaikie).

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.

In D37364#860065, @danielcdh wrote:

The change looks good to me. But the test is quite large and hard to understand. I don't think it's necessary to have the test generated from a source file, just forge a test with discriminator attached to line 0, and then checks if discriminators is omitted in the generated code. I guess it should suffice with < 20 LOC

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).

In D37038#855559, @aprantl wrote:

This may have gotten lost earlier: Would it be possible to instruct CloneFunction to not clone any temporary MDNodes via one of the flags that are passed to the ValueMapper?

In D37038#855559, @aprantl wrote:

This may have gotten lost earlier: Would it be possible to instruct CloneFunction to not clone any temporary MDNodes via one of the flags that are passed to the ValueMapper?

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.

In D37038#854722, @probinson wrote:

finalizeSubprogram() retrieves the variables from the subprogram and handles them; what is it missing?

Trying to understand the broader context here, I looked back through the list of revisions mentioned in PR33930 to see if that helped.

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.

In D6379#852974, @dim wrote:

Actually I see that we also get this warning on our .section ".note.GNU-stack","",@progbits directives, which are in most of our hand-written .S files. Since the type of that section is progbits, this fix won't work for it.

However, interestingly, clang also outputs these same .note.GNU-stack directives in .s output for compiled files, but it does *not* warn on those, even when using -gdwarf-2. So how is it avoiding them in that case?

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.

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.

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.

LGTM

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).

FTR, from a PS4 perspective this is all good, but we'd like somebody from outside our team to take a look.

I'm happy with the test changes and the code looks straightforward enough. LGTM.

In D35734#818778, @tberghammer wrote:

This section have been already removed from Dwarf5 so I agree that we shouldn't spend too much time adding support for it.

Thanks!

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.

Wouldn't you also want to verify .debug_abbrev.dwo?

Sweet. I don't have the chops to understand the Objective-C test but otherwise LGTM.

Refresh to current TOT, and ping. Funny what you can find in a year-old to-do list....

In D34685#798776, @JonChesterfield wrote:

In D34685#796778, @ruiu wrote:

I'm still waiting for you to update this patch to address Rafael's comment.

I don't see any comment by Rafael. What are you referring to?

In D34685#795512, @ruiu wrote:

Yes, that's what I meant. It's not ideal, but if you want to write tests that don't depend on target, what target would you choose?

In D34765#795663, @wolfgangp wrote:

In D34765#795633, @probinson wrote:

In D34765#795601, @wolfgangp wrote:

No you're right, my bad. Units in the .dwo sections (both type and CU) don't have a str_offsets_base, which implies that the .debug_str_offsets.dwo section has to consist of a monolithic table of string offsets (without the 8 or 16-byte header that's specified in section 7.26 of the DWARF 5 standard). Section 7.26 seems to say the opposite, though. It seems I'll have to clarify this with the DWARF5 folks.

Worth clarifying on the dwarf-discuss list but I believe the idea is that the .dwo would have a single .debug_str_offsets.dwo contribution (complete with header), corresponding to the .debug_str.dwo contribution, and all units in the compilation would share it just like they would ordinarily share the single .debug_str section in a non-split compilation.

So then given the lack of a str_offsets_base attribute in the unit DIEs the string indices in the .dwo units would have to start with 2 instead of 0 in order to ignore the header. Or else the (absent) attribute would have an implicit value of 8 (or 16 for DWARF64). Seems a bit odd.

In D34765#795601, @wolfgangp wrote:

No you're right, my bad. Units in the .dwo sections (both type and CU) don't have a str_offsets_base, which implies that the .debug_str_offsets.dwo section has to consist of a monolithic table of string offsets (without the 8 or 16-byte header that's specified in section 7.26 of the DWARF 5 standard). Section 7.26 seems to say the opposite, though. It seems I'll have to clarify this with the DWARF5 folks.

In D34685#795468, @ruiu wrote:

(Also, the fact that we are using "x86" to test generic feature means that you usually want to enable the LLVM x86 target to run these tests.)

Thanks for taking time with this!

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.

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.

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.

I'm not super excited about the hybrid form, but always unpacking a Unit inside the DWARFFormValue methods felt kind of expensive.

Ping.

A couple of minor comments inline. I'm okay with this, assuming Adrian is.

In D34177#779712, @aprantl wrote:

In D34177#779664, @probinson wrote:

I understand that DWARFAcceleratorTable already exists and you are building on that, but I have to wonder how much (if any) of this can be leveraged for the actual DWARF v5 standard's .debug_names section. (I haven't looked at any of this stuff yet; the accelerator table is pretty far down my list of v5 features to look at.) If the answer is "not much" then maybe it ought to be renamed DWARFAppleNamesTable instead.

The DWARF v5 accelerator table is a direct evolution of the Apple accelerator table, so it is quite possible that we can share some of the implementation. Form a cursory glance it looks like at least the accessors for the hash table (bucket count, ...) are similar enough that they can share a common interface. We probably can't make that call until someone actually sits down and comes up with a design for how to implement the DWARF 5 accelerator tables, though. How about we revisit this question then? Hopefully this won't be too far out into the future.

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.)

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.