Oh, and as far as dwarf aware linking I'd arguably prefer a separate tool ala dsymutil or dwz rather than incorporating it into the linker. I do agree that such a tool is incredibly useful and should be part of our ongoing development plan around debug info.

In D59553#1437670, @probinson wrote:

@jhenderson and I did a prototype of DWARF-unit-per-function last year, and I was not favorably impressed by the numbers. But maybe he still has the actual data kicking around somewhere. We did not solve all the issues to our satisfaction before we ran out of time.
I've also had chats with @bd1976llvm about fragmenting .debug_info per-function without wrapping everything in units; you get into requiring some section-order things but it saves the unit-per-function overhead.

LGTM.

& happy to be wrong here too - if we end up prototyping a "function units" equivalent of type units & seeing what the size overhead is, etc. (it'd be less problematic than type units at least - since there's no need for much/any DWARF to reference a function definition from elsewhere in the DWARF (unlike a type and its members, which all need to be referenced from elsewhere))

Per my reply to Paul I don't think this is the right way of approaching this (and neither is the linker parsing the dwarf :)

In D59553#1436890, @probinson wrote:

Does this do the right thing for a 32-bit target? 64-bit is common, but not universal.

Re. the underlying DWARF issues:
I can't remember whether the DWARF committee has ever considered specifying a special value for "address does not exist" but I doubt it, because the DWARF spec states "If an entity has no associated machine code, none of these [address-related] attributes are specified." (DWARF v5 section 2.17, p.51.) So, the current position of the DWARF spec is that the linker is supposed to rewrite the DWARF, I guess.

I can imagine a couple of ways to fix this in the DWARF spec, so that linkers can be allowed to strip entities without rewriting the DWARF, but you won't get that fix until DWARF v6 which has to be years in the future. In the meantime using "-2" is probably the simplest hack.

This is fine with me, Rui?

The duplication is a little sad, but I don't have a lot of ideas on how to remove that in the near term so LGTM. Dave, any other concerns on your end?

I'd probably prefer something around giving targets an interface rather than this... but that's a lot more work :)

In D58102#1408397, @xiangzhangllvm wrote:

Hi ruiu, very thank you for considering accepting the 2-PLT ABI design in X86 arch, Yes, we really have no real measurable evidence for the benefit of this design, and I also think there must be other problems for the CET here, but it is just the beginning. We will work on improving and refining it.
Your questions and suggestions during the discussion were all very much to the point.
I'll first to implement your first 2 review suggestions above.
Thank you again!

One more comment:

Awesome patch.

LGTM.

Hi Greg,

The way this works is feeling pretty awkward, but I think updating that will be small in contrast to getting debug working for nvptx.

If whitequark is happy I am :)

In D54674#1343910, @alexshap wrote:

@lhames , can we proceed here ?

This seems like the sort of thing that you can just commit in the future.

That's fine, it's just not the state of the world and we should support compiling appropriately.

I don't want to pick a fight, but there can be used exactly same argument to defend pulling libgcc here.

LGTM, thanks.

OK, that makes sense. I'm not a huge fan of how this set of code looks, but it also seems unfair to require you to extensively refactor it for this.

In D28791#1343013, @krytarowski wrote:

Is there intention to support Clang/LLVM by GLIBC/GNU developers? Unlikely (knowing FSF track of reactions against LLVM projects). There are no gcc/libgcc licensing concerns for software hosted on GLIBC (LGPLv3) with libgcc. There already does exist a functional implementation for this OS. There is no need to treat it as an issue on the Clang/LLVM side (maybe except ambitions).

Is something passing compiler-rt in as a -r link output or something?

Sure.

LGTM.

Thanks for the split! LGTM.

Thanks for the split. LGTM!

Thanks so much for the split!

FWIW once nemanja and Masoud are happy then this can go in.

Looks like this stalled with no feedback outside of Chandler and I.

LGTM. I'm quite a bit happier with this now. Thanks for going through the back and forth.

LGTM. Thanks!

LGTM.

LGTM still when everything else is approved.

In D54747#1315241, @ruiu wrote:

But 2GB is perhaps still too big and I guess a large part of it can be for dead sections. If we fix this, I'd like to fix it in a proper way so that we can completely eliminate debug info for dead sections.

LGTM.

In D54747#1312161, @ruiu wrote:

Thank you for the patch.

What you are doing in this patch is not too complicated and makes sense to me. That said, if actual size saving is not significant as you said in https://github.com/rust-lang/rust/issues/56068#issuecomment-440160568, it may not be worth doing. It seems to me that if debug info is already 2.4GB, shrinking it to 2GB doesn't make much difference. Do you have more convincing examples?

LGTM.

I can sign off on this.

This looks better to me. I've got an inline request for some elaboration and you might want to let chandlerc comment, but otherwise OK with me.

Test?

In D53379#1297344, @zturner wrote:

Unfortunately there's nothing else I can do on this since I don't feel enough ownership over the top level lib/DebugInfo folder to approve a new sub-folder. If you still can't get any traction from someone who does have that kind of ownership, one idea might be to make a post on llvm-dev@ pointing them to this review.

I think this is the right direction, couple comment:

In D54124#1293752, @rupprecht wrote:

In D54124#1292583, @jhenderson wrote:

Okay, from my point of view, LGTM (with a nit). However, we should probably get some more buy-in on the RFC before breaking people's builds etc, so you might want to hold off on the commit for now.

Any thoughts on how to drum up some more interest?

I'll ping the thread you started, and I'd like to commit on Monday if there are no objections. I'm hoping this change is not so controversial.

In D45784#1224910, @ABataev wrote:

In D45784#1224027, @echristo wrote:

Rather than testing for isNVPTX in AsmPrinter.cpp I'd rather just make a function "emitPreFunctionDebugInfo" and have it do nothing unless it's NVPTX and then define this in the nvptx backend. Easier to update if nvidia ever fixes this weirdness too.

Seems to me you were looking to the very first patch. The last one looks exactly like you want. Switch to the latest revision.

Once we get everything else handled this is fine :)

It'd be really great if we could handle all of this in the mips backend rather than in general code.

The llvm backend patch here has discussion around debug info kinds that we should iron out first.

Some inline comments - I'm still not happy with some of how this patch is and would like to see some changes and elaborations of how we split things out. Mostly it's bikeshed naming things, but the current state is a bit more confusing than without.

Some drive by comments again. Be good to get @ributzka to take a final look.

I have no other bikeshed colors here. :)

Naming sounds ok to me :)

In D52296#1285328, @grimar wrote:

In D52296#1284130, @probinson wrote:

In D52296#1283691, @grimar wrote:

Nice :)
So seems the last unresolved question left is the naming of the new option?
If we do not want to go with -gsingle-file-split-dwarf then what it should be?

What about -fdebug-fission as an alias for -gsplit-dwarf.
And -fsingle-file-debug-fission for the new option?

Or, may be:

-fdebug-fission for the new option and then
-fdebug-fission -fdebug-split-dwo would work together as -gsplit-dwarf?

Only DWARF supports this, correct?

I am not aware of any kind of debug information splitting except DWARF splitting.

So I would suggest: -fdwarf-fission[={split,single}] where the parameter defaults to split. Is there any particular value in having two separate options?

Probably not. -fdwarf-fission[={split,single}] sounds good for me.

In D53919#1282994, @hjl.tools wrote:

In D53919#1282952, @efriedma wrote:

With both 3.3 and trunk (I don't have a 7.0 handy; I can build it if it would be helpful):

Please try clang 2.6 on both testcases.

In D53379#1287493, @clayborg wrote:

README.md has complete file details now for anyone wanting to understand the entire file format.

Random drive-by bikeshed:

Hi Joerg,

In D52296#1246142, @grimar wrote:

In D52296#1243688, @echristo wrote:

In D52296#1241928, @probinson wrote:

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.

We do generate them generically with no objcopy hack.

Eric, could you elaborate for me your position, please

Go ahead and commit for them.

In D47811#1274645, @probinson wrote:

In D47811#1273876, @vvlevchenko wrote:

In D47811#1273052, @probinson wrote:

I've put a note out to the DWARF committee to see if we can streamline the assignment of language codes. As long as there is a source of truth for these, we shouldn't have to wait for DWARF 6 to be published.
The idea has come up before, and now with a motivating case I think it's likely to happen. So if you could hold off a little longer (probably a few days) I can have an answer for you on that point.

Ok I'll wait as long as required, for now I've updated diff regarding my understanding of our yesterday conversation. Thank you for cooperation.

Out of all the current requests to the DWARF committee for a new language code, Kotlin is the only one to request a particular number, so there's a very high probability that you will actually get the requested number.
That said, the DWARF committee does not currently have a process for guaranteeing a language code, and it may be a little while (more than days, less than years) before that's in place.
Therefore, the current state of the patch LGTM, and sorry the DWARF committee can't be more responsive at this time.

All of the target specific stuff looks fine to me. I'm going to defer to rnk about the windows side of things and aaron for the attributes.

In D52441#1271545, @rnk wrote:

In D52441#1258317, @craig.topper wrote:

Address Reid's comments. Add a comment with a list of all things that currently effect the vector width attribute emitted in IR.

For inlining, we update the caller's attribute during merging to ensure it is at least as large as the callee that is being inlined. This is required for always_inline of the intrinsics. We probably want a way to limit inlining, but that would effect the inlining decision. If the decision has been made to inline we have to take the max.

For LTO I don't have an answer. What do we do for things like target features and cpu today?

I think your comments about the behavior w.r.t. inlining are enough to describe what happens during LTO. I don't want to speak for Eric, but I think you've answered his questions.

In D53364#1271569, @wolfgangp wrote:

I just wanted to let you know that I am currently working on a patch to refactor some of the rangelists code (see: https://reviews.llvm.org/D51081) with the objective to

1. fold the pre-v5 functionality (.debug_ranges) into DWARF5 so we can get rid of DebugRangelists.{cpp,.h}
2. templatize more of the DWARFListTable usage so it could be also used for location lists
   
   Unfortunately the first submission attempt caused some problems with TSan tests that I don't have access to, so there is still some work to be done. Once I'm past this I would like to suggest basing the location list table handling on the same framework, if you are OK with that.

LGTM. This code needs a lot of cleanup, but that's for another day.

LGTM.

LGTM.