In D62202#1510449, @probinson wrote:

In D62202#1510414, @dblaikie wrote:

Technically this violates the LLVM style guide which says "make anonymous namespaces as small as possible, and only use them for class declarations." (preferring static for functions) - https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

Which argues for flagging it somehow.

Hey @lhames - is this code tested? I guess they're only debug output, so might not be tested. (was the use-after-move likely to be UB (null deref, etc) or just likely to log not what was intended?)

Is this code tested? (what happens if you replace this code with "assert(false);" and then run make check?) If not, it'd be good to add some test coverage that this has revealed is missing.

In D62202#1510394, @probinson wrote:

To provide some missing details:
The original source gets a bogus compile-time error with Visual Studio 2017, prior to version 15.8. I have 15.2, so I need this patch to build Clang.
Our current minimum-version requirement (per CheckCompilerVersion.cmake) is Visual Studio 2015 (some specific update), which assumes all Visual Studio 2017 versions are usable. So by that criterion, we need this patch for all Visual Studio 2017 versions to be usable.
Arguably the anonymous-namespace version is the "more modern" tactic anyway, so as a long-term fix this is actually fine anyway.

Is there a test case for this codepath? Or should it be an assertion instead?

Cool cool (:

Seems reasonable :)

Nice!

Seems reasonable - thanks!

Looks good - thanks!

Add another FIXME

In D61253#1491601, @MaskRay wrote:

In D61253#1491551, @labath wrote:

I believe the changes here are correct. What I find unfortunate is that it is not able to test this without including a debug_info info section in the object file. Which is a bit ironic given that the purpose of these changes in DWARF5 was to have a fully-standalone .debug_line section.

The .debug_info is used by llvm-symbolizer to locate the compile unit. Without it llvm-symbolizer doesn't bother reading .debug_line...

In D61357#1488839, @rsmith wrote:

In D61357#1485581, @dblaikie wrote:

Oh, @rsmith - if there's any better/different testing (if you can figure out how to reduce the test case down further, now that we know the cause - or if you'd like testing for other codepaths I've touched in this patch) I'm all ears. (also naming/API wrangling - my changes were just the minimal sort of "this works" based on what we discussed, but totally happy to do more involved/different things if there's such to be done)

Perhaps we could add a diagnostic scope object of some kind, which would assert (or reorder the diagnostics?) if a diagnostic is produced while producing notes for a different diagnostic. That might help for new code and for places where we're aware -- or suspect -- there is a problem, but we don't know where the existing bugs are and that doesn't help us find them. :(

In D61408#1487030, @dblaikie wrote:

Seems totally reasonable to me & it's what we do for other templates (function and class), it seems.

Might want to check alias templates too?

Seems totally reasonable to me & it's what we do for other templates (function and class), it seems.

Sorry, the title's multiple negatives confuse me a bit.

Oh, @rsmith - if there's any better/different testing (if you can figure out how to reduce the test case down further, now that we know the cause - or if you'd like testing for other codepaths I've touched in this patch) I'm all ears. (also naming/API wrangling - my changes were just the minimal sort of "this works" based on what we discussed, but totally happy to do more involved/different things if there's such to be done)

In D38061#1484568, @wenlei wrote:

In D38061#1484486, @dblaikie wrote:

Seems like the right thing would be for the DWARF code that wants a rendered type name to pass its own printing policy, rather than changing some relatively global one.

(though also I have my doubts about the whole approach - macro expansion can change the line number as well as the column number, so only suppressing column number would be insufficient - and this also reduces the usability for users (because the file/line/column of an anonymous type is a useful debugging aid). Seems to me like this should be opt-in if it's supported at all - though ideally build systems would use -frewrite-includes rather than full preprocessing, and then macros and lines/columns would be preserved, I think)

Line number is ok because preprocessor also inserts #linemarker, and later on these markers are used to recover the original line number before macro expansion. Column is problematic, as there's nothing like a column marker (if possible at all) so there's no way to see through the column change from macro expansion. We tried -frewrite-includes too, but it has its own issues - without macro expansion, it bloats the file size that needs to send to distcc remote machine significantly.

Seems like the right thing would be for the DWARF code that wants a rendered type name to pass its own printing policy, rather than changing some relatively global one.

Nah, that's all I've got.

In D60955#1474642, @craig.topper wrote:

This seems to reversing r231221 which devirtualized this. What kind of customization are you envisioning? There aren't many methods here. Are you planning to override the parse method but still fall back to the base class version sometimes?

Soundsn good to me - thanks!

Seems pretty good to me - if you'd like to wait for more/other feedback from @rsmith or the like, that's OK too.

In D60470#1469679, @MaskRay wrote:

Ah, sorry, that's not the case I was interested in. I meant a case where two object files are linked together - one has debug info, one does not. They both have a comdat function. The non-debug info version is chosen (creating the [0, X) address range in the debug info from the other object).

If the linker makes the comdat function without debug info (assuming the debug info is not in a comdat) prevailing, the one with debug info is discarded while its debug info is kept. This is essentially the same as the --gc-sections case.

The compile unit with 0 DW_AT_low_pc is actually invalid because the addresses are incorrect. It should be ignored if it overlaps with a valid range when constructing aranges. This patch will do so.

If I understand it, this change is to the prioritization/resolution of ambiguities - but similar situations/bugs can arise when there is no ambiguity (because some code has no debug info - so it's not there to create an ambiguity). if I'm understanding/explaining correctly.

Yes, it is related to the resolution of ambiguities. The old rule is to pick the CU with the smallest E.CUOffset; the new rule is to pick the CU with the largest (E.LowPC, E.CUOffset) pair.

Looks reasonable to me

Looks ok - thanks!

In D60470#1467875, @MaskRay wrote:

What happens if a program is linked from some objects with debug info and some objects without it?

This revision doesn't change the behavior. For the address ranges with no debug info, the results remain ??:0:0 (unknown).

In D60510#1468366, @ilya-biryukov wrote:

In D60510#1467262, @dblaikie wrote:

Might've made sense to separate the NFC renaming from the perfect forwarding change (& the perfect forwarding change could potentially have some matching test coverage?)

I agree, sorry about mashing them in together. That said, I was confident this wouldn't cause any trouble in practice.
What kinds of tests are you thinking about? Checking that llvm::lower_bound works on non-copyable types?

Sounds good to me.
How'd you test that the MachineMemOperand.h changes were OK? Looks like it doesn't have an implementation file, so I'm not sure it's included first in any file - which means it's harder to tell if removing headers from it is valid/keeps it standalone (since its inclusion context might be allowing it to get away without including necessary headers)

In D59673#1465975, @aaronpuchert wrote:

In D59673#1461983, @dblaikie wrote:

Sure, I think the naming's a bit weird (but hard to come up with good names for any of this)

Agreed, -split-dwarf-output is pretty clear, but -split-dwarf-file could be both the actual filename or the attribute.

There is test/CodeGen/split-debug-filename.c checking that we emit !DICompileUnit({{.*}}, splitDebugFilename: "foo.dwo" into the IR under some circumstances, but I'm not sure why. It seems to be ignored by llc.

What happens if a program is linked from some objects with debug info and some objects without it?

Might've made sense to separate the NFC renaming from the perfect forwarding change (& the perfect forwarding change could potentially have some matching test coverage?)

In D60470#1460670, @MaskRay wrote:

What does addr2line (or any other symbolizers, not that I know of others) do in these cases?

addr2line doesn't use binary search. It iterates all ELF sections, finds the first section containing the address, then iterates DWARF CUs and finds the associated line table. It has the same problem

Approved pending the LLVM side changes/discussion.

In D59673#1460605, @aaronpuchert wrote:

Ok, here is an idea. We introduce -split-dwarf-output in Clang instead of -fsplit-dwarf-dwo-name-attr. If given, it overrides the output file name for the Split DWARF file, which we otherwise take from -split-dwarf-file. The option is obviously incompatible with -enable-split-dwarf=single, so we will disallow that. This should be backwards-compatible, and bring the behavior of llc and clang -cc1 closer together. What do you think?

In D60364#1460672, @MaskRay wrote:

In D60364#1460434, @dblaikie wrote:

Should there also be an LLVM test - given that LLVM does emit DW_LNS_copy by the looks of it, and if it's been being incorrectly interpreted/dumped, then that seems to imply there's some missing LLVM test coverage for that case, maybe?

Do you mean a test of lib/MC/MCDwarf.cpp? As I understand it, lib/DebugInfo/DWARF is mostly the reader part and MC the writer part. With my casual reading, MCDwarf.cpp seems to do the correct thing by doing Discriminator = 0;. I haven't looked into that part yet but I can investigate that and add a test if needed in a separate change.

In D59923#1460696, @MaskRay wrote:

is that to imply that the first block all do not use split DWARF?

The first block do not use split DWARF.

I'm still not entirely clear on how this is all changing, sorry - in the patch description summary, the first block of examples doesn't mention which of those disables split DWARF (the second block of examples all say "+ split" - is that to imply that the first block all do not use split DWARF?

Should there also be an LLVM test - given that LLVM does emit DW_LNS_copy by the looks of it, and if it's been being incorrectly interpreted/dumped, then that seems to imply there's some missing LLVM test coverage for that case, maybe?

What does addr2line (or any other symbolizers, not that I know of others) do in these cases?

Looks good to me (: