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.

But you have other locations inside of functions so I don't get it.

Couple of comments.

Can you explain this a bit more with some examples of wrong relocations?

In D46021#1077031, @ABataev wrote:

In D46021#1077023, @dblaikie wrote:

Sorry, I'm a bit confused.

1. I'd expect /this/ behavior (line table directives, but no debug_info, etc) to be controlled via a DICompileUnit emission kind (a value between "NoDebug" and "LineTablesOnly" as Paul mentioned).
2. I'd expect the "no ', debug'" flag (which I don't see in this change? Is it missing? is it elsewhere?) to be controlled via an MCOption - because it must be global.
   
   Now, arguably, if we have the global "no ', debug'" flag we maybe might as well use it to implement this behavior too since we don't have any other use for it? So maybe (2) invalidates (1).

I thought, that it is not very useful to have an option that, actually, controls only single target and I decided instead to add the option that just disables emission of the DWARF sections. In this case we don't need the special debug info mode, we can use this option.

In D49930#1181000, @scott.linder wrote:

Sorry, I didn't see the additional comments until after I committed. I will make those changes; is it OK to update this review, or should I create a new one?

The patch is fine, in general, couple of comments:

Bit of a hack, but I'm ok with it.

LGTM.

LGTM.

Couple of small nits, but that's it. Still LGTM.

Couple of small nits, ctopper's suggestion seems reasonable too. Only "better" way I had of dealing with this would be not folding the load at all in the first place, but it seems harder to justify given the existing unfolding machinery.

LGTM.

In D49532#1169016, @dblaikie wrote:

Probably squirrel it away in the generic 'flags' bucket?

I don't necessarily think this should be part of DIBasicType since it'll be used almost never. Adding other reviewers to get thoughts on location.

Getting close, one inline comment.

LGTM. The comment is particularly helpful, thanks!

In D46230#1166958, @gaijiading wrote:

In D46230#1166919, @echristo wrote:

LGTM.

-eric

Hi Eric, I do not have commit access to trunk. Could you commit the change for me? Thanks.

LGTM.

In D49148#1166859, @echristo wrote:

In D49148#1166429, @ABataev wrote:

In D49148#1165826, @echristo wrote:

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

Thoughts?

1. I can split it, no problems.
2. Hmm, actually this what the warning already says. If the option is not supported it says 'debug option '-xxx' is not supported for target 'xxx-xxx-xxx''. It does not seem to me like a warning on non-standard option.

Let me try to elaborate a bit, I agree that I'm not very clear above.

I'm not a fan of the generic "non default debug options". It feels misleading. I think we probably want to separate it by "dwarf extensions", and "dwarf version".

As far as the error message itself: "debug option" sounds like an option to debug clang rather than a debug information option. Perhaps say "debug information option" rather than "debug option"?

It's usually preferred to submit patches with full context - it'll make this a bit easier if you do please?

In D49148#1166429, @ABataev wrote:

In D49148#1165826, @echristo wrote:

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

Thoughts?

1. I can split it, no problems.
2. Hmm, actually this what the warning already says. If the option is not supported it says 'debug option '-xxx' is not supported for target 'xxx-xxx-xxx''. It does not seem to me like a warning on non-standard option.

In D49434#1165381, @pcc wrote:

I don't think that globalopt is the right place for this because there is no guarantee that the pass is run. If we wanted to do something about this, it seems like a better place would be in lib/Analysis/ModuleSummaryAnalysis.cpp where we decide which functions are GC roots.

Also this is only a subset of the functions that the code generator can generate calls to. There is a list of runtime functions in llvm/include/llvm/CodeGen/RuntimeLibcalls.def, but as far as I know it isn't 100% comprehensive. Using that list would probably be better than just 3 functions though.

Concerned about why... but let's just do it for now and figure out the test problem later. It's not a problem with the code as far as I can tell.

LGTM. Looks like a nice set of API simplifications.

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

You'll want to wait on Rui for an actual LGTM, but this looks good to me and will match the llvm support so we can try this out in more detail and see where we should move the proposals.

Checking out the archives for this list might come in handy:

LGTM. Not a fan of adding more stuff to TargetOptions, but for now it'll do.

LGTM. Thanks and sorry for the delay.

In D48964#1158338, @ColinFinck wrote:

In D48964#1158046, @echristo wrote:

Has ELFOSABI_HERMITCORE been officially allocated?

I'm not aware of any official entity managing these ELFOSABI numbers, and have neither found references to one at the Binutils and LLVM websites.

In D49043#1163285, @paulsemel wrote:

In D49043#1163274, @jhenderson wrote:

LGTM, with the caveat that I don't know for certain if that was the original issue, without looking at the failing job and what was going on.

If you need to make minor changes to get the build bots working, no need for further review from my point of view.

I just asked @echristo if he could give this patch a try on non-x86 architecture, to be sure it was the problem ! :)

LGTM. Small organizational thoughts, but that's it.

As a quick change this looks ok, some thoughts for cleanup:

This is obviously good to start and to iterate on.

LGTM, a couple of inline requests. One small and one maybe less so.

The change looks obvious, I think the test is fine too, but MIR test cases aren't fun to read. :)

Has ELFOSABI_HERMITCORE been officially allocated?