No with testcase of sorts.

Seems reasonable, thanks!

I think this looks fine now, but I'll let Fred do the final sign off.

Thanks! Looks reasonable.

I think this looks good now.

Forgot to git-add new clang test case input files.

Note that this review contains also the corresponding clang changes in the tools/ directory.

LGTM with outstanding changes applied. Thanks!

Seems generally straightforward/fine.

This is just a general high-level comment: When adding new DWARF5 features it might also be a good idea to think about what checks could be added to DWARFVerifier to diagnose corrupt input (and potentially implement them as well :-)

LGTM, thanks!

That said, this patch is still obviously correct.

I'm slightly confused why this works as a fix for the Debug build. Isn't this patch a no-op for a debug build?

This is good to go now. Thanks!

In D34177#779723, @probinson wrote:

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.

Thanks for the info, if there's a reasonable chance to make it reasonably common then I'm happy to leave the naming as is for now.

In D34177#779664, @probinson wrote:

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

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.

Hm.. I think the other commit in the blamelist looks far more promising.

It looks like this broke the stage2 build:

Looks good. Please watch the stage2 and LTO bots carefully after committing.

IIRC git diff -U9999 should work just fine. In any case I am not going to review the binary file — I am just going to trust that it is what you say it is in the accompanying testcase :-)

This generally LGTM, there is only a bitcode upgrade test for DW_OP_minus missing. I'm marking the review as accepted under the condition that the missing test is added before committing.

Technically this is missing a DWARF backend test, but since the next review will rewrite all DW_OP_plus to DW_OP_plus_uconst anyway, this is fine.

Ah ok, I see. The cleanest way to do this would be to

1. Implement support for DW_OP_plus_constu in LLVM
2. Switch over Clang to use DW_OP_plus_constu

3a. Retire the old DW_OP_plus and implement the bitcode upgrade in LLVM
3b. Retire the old DW_OP_minus (AFAIK not used by clang) and implement the bitcode upgrade in LLVM
(The last two can be one joined commit so we only need to bump the DIExpression version number once and keep the upgrade code simple).

Thanks! I noticed that in the remainder of the patch there are still references to DW_OP_plus left, but they are completely untested. I would recommend removing all referenced to DW_OP_plus in this patch and then adding support for the "real" DW_OP_plus back in a separate commit, together with testcases.

Almost there! Couple more comments inline.

Thanks, this LGTM now.

It looks like you accidentally created a diff between your last patch and the new version. When uploading a revised patch, please always upload the diff from svn trunk.

This also needs a bitcode upgrade to transform the old use of DW_OP_minus to the new one. See my comment on D33892 for details.

Thanks, this LGTM!

Thanks, this is great! The only thing that is missing here is a bitcode upgrade that transforms the old use of DW_OP_plus to DW_OP_plus_uconst. I recommend incrementing the "Version number" in operand 0 of DIExpression to distinguish between the old and new format. You can look at r300522 for an example of how to do this.

Thanks, I made a couple of comments inline.

Looks good.. Are you also planning to change DIBuilder to not finalize subprograms automatically any more (and not insert them into AllSubprograms)? (That will be the more impactful change as it will force all non-clang frontends to make a similar change).

Thanks. I can commit this for you.

I'm fine with this change. I'm fine with either expecting the frontends to manually call finalize on each subprogram or with having DIBuilder do it. But let's have that discussion over in https://reviews.llvm.org/D33705.

https://reviews.llvm.org/D33705 is the clang part of this. @dblaikie had some concerns (on the mailing list) - I had it only call it on cloned subprogram, but @dblaikie thought it might be more natural to call it immediately when irgen is done.

I was a bit scared by the description at first, but this is safe. Will you only invoke it on functions to be cloned in clang or on all subprograms?

Thanks Keno, this appears to be a far better approach. The only question that remains is whether / how to update the linkage name of the cloned DISubprogram. But I don't think that this needs to be solved in this patch.

This is good from my point of view now. You might want to wait for an ok from @spyffe.

In D33370#759915, @mehdi_amini wrote:

It turns out that the SrcModule in FunctionImporter is in a really inconsistent intermediate state at the point where I ran the Verifier.

Oh right I forgot about this: lazy-loaded modules, even after materializing metadata, aren't passing the verified. This is super annoying.

After poking around a bit I came to the conclusion that it probably isn't even desirable to run the full-module verifier on each cross-imported module: Over the course of building the entire project the same module will be verified over and over again, which is a waste of resources. When we want to lazy-load metadata running a whole-module Verifier is also problematic.

There is something I'm missing: I thought the important part of the verifier was to be able to drop stalled debug information which is important for backward compatibility? This is the only reason I suggested doing it at this point of the process (before importing)...

(Having unit tests will avoid

Thanks for the review!

Depending on https://reviews.llvm.org/D33360, get rid of the verifier invocation in codegen()

In D33151#756851, @aprantl wrote:

In D33151#756801, @mehdi_amini wrote:

It is likely before run() was improved to be able to handle CodeGenOnly, we should retarget llvm-lto to use run() instead. I can do this if you want (likely tonight or tomorrow night)

In D33151#756801, @mehdi_amini wrote:

In D33151#756793, @aprantl wrote:

In D33151#756788, @mehdi_amini wrote:

It seems to me that it indicates that the check in the codegen() method is redundant now. Why do we need it? What about just removing it?

Because llvm-lto invokes codegen() directly after loading the module itself (wihtout going through ThinLTOCodeGenerator for the loading):

It is likely before run() was improved to be able to handle CodeGenOnly, we should retarget llvm-lto to use run() instead. I can do this if you want (likely tonight or tomorrow night)

In D33151#756788, @mehdi_amini wrote:

It seems to me that it indicates that the check in the codegen() method is redundant now. Why do we need it? What about just removing it?

Fix typos.

This assumes that there exists a main module, which is a foreign concept to ThinLTOCodegenerator till now AFAIK. I'm not sure what you're trying to express here?
Especially, this seems used only in the codegen, which is only use in a very specific mode (i.e. split model where you store optimized bitcode and reprocess to perform only the codegen). In such case either you want to verify all the modules that you're about to codegen or none. I have the impression that you'll check only the first one. Not sure if I missed something.

What an entangled mess. This patch adds a boatload of testcases including the case where a function with broken debug info is cross-module-imported into a verified correct module. It also avoids re-verifying the module as much as possible.

Yes I think you are right. There are too many places that assume the Verifier is as function pass.
The updated patch disabled input verification in the pass manager builder and manually verifies the input.

I'm assuming the checks are removed because they are already done elsewhere?

This turns out to be quite ugly.
There are two different code paths, by their llvm-lto options:
-thinlto-action=run uses PassManagerBuilder and sets VerifyInput=true, which causes a VerifierLegacyPass to be scheduled
-thinlto-action=codegen (which mimics llc) doesn't.
The obvious correct fix for -thinlto-action=codegen is to also schedule a Verifier pass. That is easy — but wait.

Interesting. The linker invocation I was debugging was using the

I don't have an informed opinion at the moment, besides "let's benchmark it", so please go ahead for now. Once we switch over LLDB to use the LLVM DWARF parser I will likely revisit this in more depth.

I don't think that is going to work, because of the report_fatal_error that also exits.

This looks generally good (with outstanding comments addressed), thanks!

Interesting, right!

This does not really have anything to do with the code in this patch though. I just really want to understand it.

I totally understand that it doesn't need to be tracked in the MMI table and in a DEBUG_VALUE, my question was that it sounded to my like you implied that the DWARF backend actively ignores DEBUG_VALUEs pointing to static allocas which I found really surprising (as there are legitimate reasons for allocas to be described by a DEBUG_VALUE) .

Is it the right solution to use the module hash for correctness, or should the mismatch of the serialized langopts trigger a module rebuild and the module hash is only there to tune the performance/disk size tradeoff?

An llvm.dbg.declare of a static alloca is always added to the
MachineFunction dbg variable map, so these values are entirely
redundant.

David, are all your concerns addressed?
Otherwise from my end this change LGTM (with all outstanding issues addressed).