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.

Feel free to make any follow on changes you need here.

I think Adrian has looked at this more recently than I have. Adding him here.

LGTM. Thanks!

The specific use of ENUM_ENT is also somewhat confusing, but you didn't add that so OK.

LGTM.

I'm ok with the "here are a bunch of fuchsia tests" file :)

LGTM as well. Thanks!

SGTM.

LGTM.

Couple of comments/questions:

Sounds like everyone is in favor. Hitting the accept button for Jordan :)

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.

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.

LGTM.

Adding Steve here in case he wants to opine.

I'll get it tomorrow if no one does before.

LGTM

Meta comment: please split the rename out. I don't mind the rename, but it makes the change much harder to review.