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.

Do you mean basically just line tables or something else? (Minus the need in normal dwarf for a minimal cu to associate the line table with). Also, how does this handle inlining etc?

The change in name here from "line tables" to "directives only" feels a bit confusing. "Limited" seems to be a bit more clear, or even remaining line tables only. Can you explain where you were going with this particular set of changes in a bit more detail please?

Some inline comments.

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.

In D47104#1219760, @aprantl wrote:

My *guess* is that the underscore trimming was added to allow setting breakpoints by name on labels in assembler output generated by a compiler, but I'm not sure. Perhaps @echristo knows more?

LGTM, but let's get Stephen to ack as well.

Feel free to add any tests like this that test existing behavior without review in the future. If we want to change the behavior we should probably review that though :)

Yeah, that works for me. If we start getting something wrong it's an edge case we should probably document :)

LGTM

Going to guess you need someone to commit this for you.

Should we just have them mean the same thing and change it based on target?

LGTM. Thanks.

Oh, you probably want to fix the commit message here.

SGTM.

LGTM. Thanks.

LGTM. One inline comment.

In D50099#1189667, @scott.linder wrote:

When I went to mark these as static I noticed they use CGDebugInfo::CreateMemberType which uses a couple other non-static member functions, and it starts to become difficult to tease things out into nice clean static functions.

Looks pretty good. Could you pass CGM in and just make the functions static I couldn't see any other class variables, but might have missed something. One inline comment as well.

In D49148#1184957, @tra wrote:

I wonder, what's the right thing to do to silence the warnings. For instance, we compile everything with -Werror and the warnings result in build breaks.

Easy way out is to pass -Wno-unsupported-target-opt. It works, but it does not really solve anything. It also may mask potential other problems.

Another alternative is to change clang driver and filter out unsupported options so they are not passed to cc1. That will also work, but it looks wrong, because now we have two patches that effectively cancel each other for no observable benefit.

Third option is to grow a better way to specify target-specific sub-compilation options and then consider fancy debug flags to be attributable to host compilation only. Anything beyond the "safe" subset, would have to be specified explicitly. This also sounds awkward -- I don't really want to replicate bunch of options times number of GPUs I'm compiling for. That may be alleviated by providing more coarse way to group options. E.g. we could say "these are the options for *all* non-host compilations, and here are few specifically for sm_XY". I think @echristo and I had discussed something like this long time ago.