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

By the way: you probably noticed that since last week have a brand new DWARF Verifier (accessible via dwarfdump --verify). It might be nice to develop Verifier checks alongside with the support for new sections/formats.

Seems generally fine, couple of suggestions inline.

Thanks!

Ah. Thanks for clarifying! We should remove it, then.

I'm not sure if globals are still reachable via !dbg, but it certainly used to be possible. Adrian, do you know the current state?

I'm not sure I understand your question:

Sure. I'm fine with a separate commit that breaks the Verifier out into its own file and also breaks the current HandleDebugLine function (which btw. should start with a lower-case character) into smaller functions.

Thanks! I'll leave this to David to mark as accepted.

Doesn't the test need some kind of REQUIRES: mips line?
Otherwise looks fine.

I think this looks much nicer!

LGTM once outstanding issues are addressed.

seems generally fine, couple of stylistic nits inline.

Address further review feedback from David.

Don't make anonymous namespaces distinct.

First off, thank you very much for getting this started!

Added a negative check for decl_line/decl_file to DebugInfo/Generic/namespace.ll

In D32648#740966, @dblaikie wrote:

I'm all for this, couple of questions though:

• When would the use of "distinct" on an anonymous namespace matter? Since LLVM uses cross-CU references to refer to entities in different CUs, never importing/moving entities between CUs, the anonymous namespaces shouldn't collide, should they? (anymore than named namespaces would collide when there are different entities in different CUs but both in the same named namespace - LLVM still produces two separate namespaces, one in each CU, one with each of the respective entities)?

Add a bitcode upgrade for anonymous namespaces.
Thanks, Duncan!

In D32618#740265, @wolfgangp wrote:

In D32618#740256, @aprantl wrote:

Also, apologies for not handling Mach-O.

Typically we try to do a best effort on all supported containers (ELF, Mach-O, COFF) when adding new features.
Could you outline what is necessary there? Is it just coming up with a short-enough section name or is there more to it?

That is probably it. Basically, the hardest part is just making string references unique to the unit (compile unit, type unit) and putting all the relocatable string references into the new section. Nothing that should be specific to the object file format. I can take a stab at it, it shouldn't take too long.

Also, apologies for not handling Mach-O.

Typically we try to do a best effort on all supported containers (ELF, Mach-O, COFF) when adding new features.
Could you outline what is necessary there? Is it just coming up with a short-enough section name or is there more to it?

Thanks! The patch may be large, but the changes are mostly straightforward and generally look good to me. I found a couple of nits inline.

I think I may haved screwed up the phabricator process for this one.
This landed as r300523.

It would simplify fallout, reverting, triaging issues, understanding compile time impact, etc., if the debug info change was left for a separate commit (maybe waiting a day). Thoughts?

Address review feedback.

@beanz: Would sorting all these set() commands alphabetically break anything?

Thanks!

r301498

Expand testcase to use more than one thrown type.
Wire up DIBuilder support & add a unittest.

@echristo: Is there anything besides LLVM tests that depends on llvm-dwarfdump's output ordering for the .gnu_pubnames section?

Could you also add some unit tests for the new interface?

ok.

Besides my above comment, I'm very much looking forward to this commit. It will also help a lot with implementing dwarfdump -verify functionality.

LGTM with inline feedback addressed. Thanks!

Rather than scanning forward for DBG_VALUEs, is there a more systematic way to find them? E.g., by iterating over USEs? If not, doing it this way is probably fine.

Address review feedback. Thanks!