I see some unrelated whitespace changes, we generally don't like mixing those with "real" changes. But the description seems fine to me.

In D126224#3532769, @dblaikie wrote:

While the string would be deduplicated, the offset in the DIEs (or index in DWARFv5, plus offset in .debug_str_offsets) for each of these strings would not. But perhaps that would still be low-cost enough (if we were getting really fancy, we could bake that string offset/index into the abbreviation - though we probably don't have the infrastructure in LLVM's DWARF emission to do that super easily)

Unnamed variables are an oddity, sure; we've had to patch a downstream test or two that wasn't being careful enough. But not providing a name is entirely defensible, and consumers should be willing to cope with DWARF that doesn't fully meet their expectations.

In D124082#3495415, @JDevlieghere wrote:

In D124082#3474691, @probinson wrote:

Comparing different DWARF output in a more abstract way is exactly what the llvm-dva tool is designed to do; unfortunately it isn't upstream yet, although @CarlosAlbertoEnciso is working on generating the patches now.
I'd prefer not to have a works-in-a-few-cases-for-one-tool feature come in when a better solution is around the corner.

+1 if that tool is actually coming. @CarlosAlbertoEnciso or @probinson do you have a rough timeline in mind?

The buildbot totals are counting the number of shards across all unit tests. The summary at the end of the log is counting the number of test cases like before.

This feels like (maybe is?) supporting a target whose assembler happens not to be generally distributed, but which LLVM still supports as a target. For those environments/bots that actually have the assembler installed, having that extra level of validation seems worthwhile.

Lit is aware of zlib's presence or absence, you could use REQUIRES: zlib (or you might factor that bit out into its own test with the REQUIRES).

Comparing different DWARF output in a more abstract way is exactly what the llvm-dva tool is designed to do; unfortunately it isn't upstream yet, although @CarlosAlbertoEnciso is working on generating the patches now.
I'd prefer not to have a works-in-a-few-cases-for-one-tool feature come in when a better solution is around the corner.

I don't spend a lot of time mucking around with IR transforms, so I'm not clear how _preserving_ the debug instructions makes it _possible_ to delete the block. Intuitively that doesn't make sense.

I'm okay with doing it this way. My impression over the years (as a debug-info guy, not as a ptxas user) is that ptxas evolves slowly and this is not really "busy work."

LGTM

What's the DW_AT_type referring to? (why is there a type called ".str"?)

Offhand, it seems like the next step would be a patch to make the update scripts apply --allow-unused-prefixes to all FileCheck commands. Then all autogenerated scripts would (eventually) DTRT, and this patch could then be reverted.

In D123897#3456962, @lebedev.ri wrote:

Apologies, it seems like the feedback here arrives much faster than usual, so there are merge conflicts.
If the current description is still seen as bad, here's a black marker, feel free to fine-tune it.
Thanks!

In D123319#3437250, @dblaikie wrote:

(@probinson as someone I've disagreed with about this before)

Personally I think there's limited value in expressing 'auto' in DWARF at all - we could omit function declarations if the return type is not known (undeduced auto) and wouldn't lose much - basically treating them the same as templates that aren't instantiated yet.

In D122584#3434184, @dblaikie wrote:

Oh, this goes back in time a bit of a ways indeed (thanks for the archaeology) - so there was probably no pre-commit review, and the post-commit review doesn't seem to have discussed this aspect of the design, just some other naming details: https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20121203/thread.html#158001

OK. It is indeed a small obvious fix.

I really like these results but I don't feel confident enough about Python to give a final okay.

I see that for %if feature {A} %else {B} the space before the %else is not preserved in the result.
If there is no %else what happens? e.g., XX %if feature {YY} ZZ Asking because I see no test case for this.

+cfe-commits; that should have happened automatically?

Relevant quote from Options.td:

def gsimple_template_names : Flag<["-"], "gsimple-template-names">, Group<g_flags_Group>;
def gsimple_template_names_EQ
    : Joined<["-"], "gsimple-template-names=">,
      HelpText<"Use simple template names in DWARF, or include the full "
               "template name with a modified prefix for validation">,
      Values<"simple,mangled">, Flags<[CC1Option, NoDriverOption]>;
def gno_simple_template_names : Flag<["-"], "gno-simple-template-names">,
                                Group<g_flags_Group>;

My suspicion is that most peopke just run check-llvm or check-clang, which includes the regular lit tests, and therefore already ends up with things split across he available cores.

In D122251#3401008, @ychen wrote:

Do we support using upstream googletest instead of the in-tree copy for the unit test?

In D122003#3399214, @rengolin wrote:

My original point was that this change is so small and unrelated to anything at the moment that it would be better for whatever s390x or i386 change that needs it, to introduce it then, not beforehand.

So this review should be abandoned in favour of the actual s390x review that needs it, with this change in it.

I traced it back to 2011 (commit 40f8f2ff "Add support for a new extension to the .file directive") , where it it was added as an extension and (I assume) later included into DWARFv5. Do we still want to support it as an extension for DWARF < 5, or it is only needed for integrated assembler?

What I've seen done in other cases is to write a second test file that uses the first test file as input. This lets the second test file have different requirements than the first. In the example here, you wouldn't modify access-non-generic.ll but rather add a second file, say access-non-generic-ptxas.ll that contained just this:

REQUIRES: ptxas
RUN: llc < access-non-generic.ll -march=nvptx -mcpu=sm_20 -o %t.ptx
RUN: ptxas -c %t.ptx -o /dev/null

The upside is, this uses existing lit mechanisms (feature test based on discovery of "ptxas" on PATH, or some environment variable is set).
The downside is, a bit more effort to keep the two files consistent; although in this case, it looks like the "ptxas" commands are pretty simple so I shouldn't think it would be a real burden.

You still need a test for this change. It seems there isn't really an existing test for this API; it's used in llvm/unittests/MC/TargetRegistry.cpp, but that's testing the registry rather than the API.

Test? I'm a little surprised that changing the spelling of an arch didn't cause any failures.

Makes sense, LGTM.

We had the same thing happen downstream. One of our guys speculates that because some allocations are "owned" by the returned TokenStream, but the returned TokenStream is a temporary, the lifetimes might not be sufficient and some deallocations can happen.
If I change the nested calls stripComments(cook(lex(Code, Opts), Opts)) to separate calls stored to separate locals, the problem goes away.

LGTM, sorry I missed this!

In D111587#3383684, @keith wrote:

You're right it's version 5 not 4, maybe the issue is that some platforms (like macOS) are defaulting to 4 intentionally for now? I guess I thought 6 because passing 6 also reproduces, but I didn't also try 5.

In D119040#3363406, @kadircet wrote:

Thanks for the change, but the test is actually checking for rename in presence of using-decls. I beleive https://reviews.llvm.org/D121103 is the proper fix here.

In D111587#3382834, @keith wrote:

I actually mean dwarf 6, which appears to be partially implemented according to https://lists.llvm.org/pipermail/llvm-dev/2020-April/141055.html

I discovered the issue from the failed tests on https://reviews.llvm.org/D113718 where you can see the test output contains a checksum that otherwise doesn't appear. Passing -dwarf-version=6 reproduces the issue.

In D111587#3381369, @keith wrote:

Fix tests with dwarf 6

+ @hokein who has done work in the one place where replaceNestedName is used.

I think the encoding of the line table may make it infeasible to walk backwards. But perhaps the code that does interpret the line table could keep track of the most-recent-nonzero-line and return that in addition to (or instead of) zero, when the requested address does resolve to zero? If it's instead-of, the API would have to return an indication that it was approximated, so this could be displayed appropriately.

Incidentally I agree with @jhenderson that there should be some indication in the output that a result is approximate.

The decrementing solution came up because in the cases where we see this in the wild, it's symbolizing a backtrace (or possibly just one return address) and the return address naturally doesn't point to the call instruction, it points one past the call instruction; the instruction after a call might have line 0 for various reasons, the most blatant being that it's a noreturn call. But the call instruction itself nearly always will not have line 0.
So, _in practice_, we'll basically decrement once and be done.

Seems to me there have been other cases where .* matching was greedier than expected, so this seems like the right fix.

Ping; + @bkramer who reviewed the original patch that added this test.

The ability to enable/disable accelerators on MachO (not currently possible - since it's hardcoded in the backend) - eg: someone adding -gno-pubnames to reduce debug info size, but in the process breaking lldb's behavior due to missing accelerator tables.

Ping; +@kadircet