TL;DR: It's all good.

In D68620#2276034, @Orlando wrote:

(as an aside: do you/@probinson have much of a sense of how much binary size increase you'd trade for object size reductions in this space? If binary size is ulmtimately what you care most about, I'm guessing maybe debug_loc base address specifiers will never be a win for you & perhaps we should just group them under a flag, maybe refactor the debug_ranges base address specifier flag to cover both (the flag there was introduced due to a gold+gdb_index+32 bit binary bug, unfortunately, but lumping them together seems OK-ish to me))

I'm not sure, I'll defer to @probinson on that.

In D68620#2266531, @dblaikie wrote:

@probinson - do you have any state here? Whether this is an ongoing issue, whether the improvements to avoid unnecessary debug_loc have reduced the overhead sufficiently, etc?

Couple of nits and LGTM.

Whoa--that's the *help* text? Well, if that's the only documentation for options that there is, I guess it has to go there; but it seems pretty excessive for the (ideally concise) output of clang --help.

Looks okay (one grammar nit), probably worth waiting for someone else to chime in.

No test?

@rupprecht if you are still motivated to land this after so long, the only thing I see missing is to update the example from CHECK-SAME: 1{{$}} to CHECK-SAME: {{ 1$}} and then it LGTM. If not, there seems to be enough interest that someone else can land it?

LGTM

It looks like you have been tweaking a number of related SizeOf() methods. Is it practical to have just one? In a parent class, or even as a free function (as the size computation depends only on form/format, and is not related to the class at all).

I agree with @grimar as it looks like the change is from a DWARFDataExtractor without relocations to one with relocations.
Regarding header order, in cases like this it's usual to do an NFC commit to sort the headers that are there, and then the functional patch just adds the new one in the right place.

Just wanted to say I agree this can be better than the Inputs/ idiom, especially for comparatively short inputs. We have some downstream tests that use a bunch of echo commands to generate a second file, but an extract utility would be cleaner IMO.

According to https://bugs.llvm.org/show_bug.cgi?id=46653#c3 there is no problem on x86-64, suggesting the fix is more likely to be in the AArch64 target instead of in generic code. It's not uncommon for passes to lose source locations, or fail to add them when they should, so that is more likely to be the cause of the AArch64 issue.

Wouldn't using .ifdef directives with --defsym on the command line work equally well? At least for .s files run through llvm-mc.
There are examples of this in the lit tests already, easy enough to find.

LGTM.

In D83462#2142036, @echristo wrote:

So the tuning here for SCE is also a "does not support" or something else?

Between this code and DwarfDebug, I'm having a hard time understanding the various controls over emitting entry-values.

• There's an existing tuning check in DwarfDebug, now you're adding the complementary check in TargetOptions. Does this make the DwarfDebug check redundant?
• There are 3 different flags to sort out whether to emit this stuff. SupportsDebugEntryValues is strictly based on the target, I get that. Then there's EnableDebugEntryValues in the TargetOptions, which seems to be tied to a tool-command-line option; and separately there's EmitDwarfDebugEntryValues in DwarfDebug, with its own command-line option. Can we simplify this at all?

Thanks! This will keep the debugger folks off my case. :-)

In D82975#2134132, @dblaikie wrote:

In D82975#2128353, @dstenb wrote:

In D82975#2127201, @SouraVX wrote:

I think if it's about compatibility(analogous behavior with GCC), existing infra is Okay/Fine(Since same encodings are used). We just need to emit the .debug_macro section with version 4 and teach the llvm-dwarfdump to parse it correctly.

One difference though is that the GNU extension does not have anything like the strx entries that LLVM currently emits: https://github.com/gcc-mirror/gcc/blob/master/include/dwarf2.h#L425, so I assume we still need code to emit the strp entries when targeting DWARF 4?

Likely - but might want to check what GCC does - maybe it uses some kind of strx encoding that's not documented, etc.

I think I didn't fully grasp that the blocks were being (tail-)merged, which makes the scope ambiguous, and all the rest. So I withdraw the objection on that basis. DWARF is fine with multiple variables pointing to the same location, but it's less forgiving about scopes IIRC, much like it can't describe multiple source attributions for an instructions. This all makes me sad, but that's how DWARF is at the moment.

I agree that being able to set a breakpoint on each different source of an ICF is useful, and that the COMDAT case typically has the same source and can use the tombstone.

In D82129#2114150, @Orlando wrote:

Looking closer at COFF/register-variables.ll, it doesn't look like a bug but instead another victim of how we model debug info. Before running -branch-folder (Control Flow Optimizer) all the instructions in the else block belong to the else block scope. The branch folder merges the common tails from 'then' and 'else' into 'else', merging the debug-locations while it does so. @jmorse summarised the situation well offline: Every time we call getMergedLocation, we are creating the conditions where this occurs, and eliminating it during compilation would be a high burden.

In D82129#2110934, @Orlando wrote:

In D82129#2107017, @probinson wrote:

This patch depends on the ranges for all scopes to be (reasonably) correct,

I'd say instead that 'variable locations depend on the ranges for all scopes to be (reasonably) correct'. And that this patch just acknowledges that relationship and clears away what we cannot use/see in a debugger.

Either way, the question remains: when we find cases where we need to "clear away" something, is that a bug, or is this merely a cleanup pass? In the case of the test I commented on, it's a bug, and I'd rather not be hiding bugs.

In D82363#2108840, @StephenTozer wrote:

In D82363#2108817, @djtodoro wrote:

DBG_VAL_VAR stands for variadic, any alternative for the name? VAR may indicate to 'variable' I think...

DBG_VALUE_VARIADIC seems reasonable though also quite long, but it's better than being misleading.

This patch depends on the ranges for all scopes to be (reasonably) correct, but I think there's one modified test where that's not the case; a variable location is dropped because (AFAICT looking at the equivalent DWARF output) its containing scope isn't pointing to the right instructions.
If we take the stance that these cases are bugs, then this analysis is better off done in a verifier, so we can find and fix those cases, rather than papering over compiler bugs.

I've been watching, and have nothing to add to what James or Dave had to say.

We recently ran into a case where it looks like the variable's location was correct but the containing scope was wrong. I guess we'll need a separate update for how to manage scopes.
Not an objection to this patch, just observing there are more cases to think about.

In a .test file, you don't actually need the comment markers. They're not harmful either, so up to you whether to leave them in.
Some people prefer that commentary and action lines (REQUIRES, CHECK, etc) have different comment markings, just for readability.

Convert to using cursor instead?

Definitely better, thanks!

In D79507#2093088, @DavidTruby wrote:

I fully support having a requirement that LLVM should support being shipped in Release with LLVM_ENABLE_ASSERTIONS configurations as a first class citizen, and would support an RFC on this on the LLVM mailing list.

LGTM with one inline comment.

In D81469#2084372, @grimar wrote:

In D81469#2082410, @probinson wrote:

If the .eh_frame section contains contributions for multiple compilation units, the dumper should skip over the bad one and continue dumping at the next one. In which case, you'd need only one test, with multiple units in it (imitating a linked executable).

Sounds reasonable.

But Error DWARFDebugFrame::parse(DWARFDataExtractor Data) returns errors for a few cases already,
e.g. for the case when something is wrong with the augmentation string. Instead it could provide a way to
report a warning (perhaps a warning callback) and continue dumping.

Reimplementing the DWARFDebugFrame::parse() to support warnings should be done independently from this change I think?

Without this patch, I'm seeing line 109 reported, which makes no sense at all. The test shows with the patch we get line 105, which in effect means that the line directive is processed as if it was not inside a macro definition; that makes some sense.
LGTM

In D81144#2089059, @ikudrin wrote:

Ping. Are you OK with this patch and the whole series or there is something I should improve?

LGTM.

+debug-info tag

In D59553#2083362, @dblaikie wrote:

Ah @MaskRay posted the link to the DWARF issue ( http://www.dwarfstd.org/ShowIssue.php?issue=200609.1 ) in another thread.

I've filed a DWARF issue to reserve "-1" as the tombstone address value. I wrote it up such that consumers should ignore that address and any address derived from it (e.g., base+offset). I don't have the issue number for it yet, hopefully I'll remember to post it here when it's assigned.

If the .eh_frame section contains contributions for multiple compilation units, the dumper should skip over the bad one and continue dumping at the next one. In which case, you'd need only one test, with multiple units in it (imitating a linked executable).

In D81144#2081485, @ikudrin wrote:

If an assembler source contains .file/.loc directives, and -dwarf64 is given, llvm-mc will still correctly produce a DWARF64 .debug_line section. That is because the only differences between DWARF32 and DWARF64 ones are unit_length and header_length fields, and references to .debug_line_str, and all these are covered by this patch.

Do you have any specific example which results in an incorrect output with these patches?

That's a pretty long test case for a one-line patch; there's no existing zext testcase where you can just add !dbg metadata?

I don't remember the exact reasoning but I believe it had something to do with bot logs? @jdenny or @thopre might remember.

In D59553#1627199, @avl wrote:

@echristo @aprantl @probinson

Folks, I would like to ask a question related to this review and DWARF standard:

DWARF5 standard has the following statement related to non-continuous address ranges:

2.17.3 Non-Contiguous Address Ranges
...
A bounded range entry whose beginning and ending address offsets are equal
(including zero) indicates an empty range and may be ignored.

Would not it be useful to extend it to explicitly explain the following case
(for some next version of the standard)

"A bounded range entry whose beginning address offset greater than ending address offset
indicates an invalid range and may be ignored. "

?

This situation (LowPC > HighPC) already represents an invalid range.

llvm-dwarfdump --verify :
error: Invalid address range [0xfffffffffffffffe, 0x0000000000000004)

Thus it would be useful to indicate that explicitly and describe that default handling would be to skip.
So that broken ranges do not affect valid ranges.

For this patch, it would mean that practically all cases
would lead in this situation, and DWARF readers would adequately skip them :

LowPC -> points to deleted section -> set to UINT64_MAX-1; 
HighPC -> points to deleted section -> set to UINT64_MAX-1; 

ignored.

LowPC -> points to deleted section -> set to UINT64_MAX-1; 
HighPC -> points to correct section -> set to some value less than UINT64_MAX-1;

ignored.

LowPC -> points to deleted section -> set to UINT64_MAX-1; 
Length has some value -> HighPC = UINT64_MAX-1 + value -> overflow -> less than LowPC

ignored.

What do you think?

(By "hand-written assembler" I mean assembler source without .file/.loc directives. llvm-mc is perfectly capable of assembling a file with .file/.loc directives but that will take different paths not covered by this patch. It was not at all clear from the patch description that only the former case was intended.)

I accept that generating DWARF64 format for hand-written assembler source has use-cases.

FTR, we like to have full context diffs as described here: https://llvm.org/docs/Phabricator.html#phabricator-request-review-web

In D81145#2074793, @dblaikie wrote:

In D81145#2074692, @probinson wrote:

This patch is for generating DWARF-64 format for raw assembler source. Why is that an interesting case? I could see wanting to emit a .debug_info section for higher level languages in DWARF-64 format, but that is not what this patch does.

Might be best to keep the fundamental design direction/question here in one thread ( https://reviews.llvm.org/D81144 ) rather than in all of them. Patches are in a series, so if that one ends up going another direction I don't think there's a risk these later ones will be reviewed/approved/committed in spite of that fundamental question.

So if we want DWARF-64 .debug_aranges for both asm source and higher-level language source....
This patch only does the asm source path, not the main path. I am not 100% clear where the other path is, but I see code to emit .debug_aranges in lib/DWARFLinker/DWARFStreamer.cpp, is that where the normal path is now?

Ah sorry hadn't noticed this one was abandoned. Never mind.

This patch is for generating DWARF-64 format for raw assembler source. Why is that an interesting case? I could see wanting to emit a .debug_info section for higher level languages in DWARF-64 format, but that is not what this patch does.

The test is verifying that llvm-mc can emit a DWARF-64 line table describing assembler source that does not have any .loc directives in it. That's not really the interesting case; the interesting case is assembler source that *does* have .loc directives in it, because that's the path that most compilations will actually use. Describing assembler source really doesn't need to support DWARF-64.

Some of these changes are specific to cases where we are emitting DWARF describing assembler source; I don't see any reason to support DWARF-64 for those cases. (In general, any functions with GenDwarf in the name are for this case.) Also in most cases the DWARF version for sections describing assembler source are hard-coded to DWARF version 2, which did not define the DWARF-64 format.

This patch modifies EmitGenDwarfAranges(), which is used only when the assembler is generating DWARF to describe the assembler source.
I can't imagine anyone is really writing large enough assembler that they need to emit DWARF-64 format. What is the motivation here?

The correct fix is to emit the .debug_line.dwo section (which repeats the directory/file tables from the .debug_line section in the .o file) and have .debug_macro.dwo point to it. "If a DW_MACRO_start_file entry is present, the header contains a reference to the .debug_line section of the compilation." (p.169) Yes, it would be clearer to mention both normal and .dwo cases, and I will file a clarification issue about that, but I think the intent is clear.

Re testing, you could copy clang/test/Driver/at_file_win.c, which has an explicit --rsp-quoting=windows; remove that option and make it REQUIRES: system-windows. That should do it.

It looks like your patch will allow us to remove a private patch that has a similar effect.
Incidentally if I apply this to an upstream checkout on Windows, I see clang/test/Driver/at_file.c fails, so you'd at least need to do something for that.
(UNSUPPORTED: system-windows at a minimum.)

I was about to add @rnk who I remember has been in Windows driver behavior discussions in the past; except he has cleverly left a note that he's away June-Sept. Oh well.

Indeed I am mis-remembering, sorry about that! We use gnu-style command-line options but we change RSPQuoting from Default to Windows; assuming getProcessTriple() is the host not the target, your change should have the same effect. I'll try removing our private patch and adding yours, and see what happens.

As a functional change it should definitely have a functional test.
Let me double-check on how our stuff behaves, I may be mis-remembering.

In D80242#2045362, @nickdesaulniers wrote:

For C++, I'd imagine:

class foo {};
using my_foo = foo;
template <typename bar>
struct baz {
  bar my_bar;
};

but it seems that g++ doesn't emit debug info the for the templated struct.

LGTM with the suggested helper.

Fixed by llvmorg-11-init-16204-g8c2d2d971b2 (forgot to put the revision in the commit log).

I believe GNU readelf and friends tend to put this info after the length, e.g. "Length: 0x1234 (64-bit)" or "Length: 0x1234 (32-bit)" so it might be worth being vaguely similar.

LGTM

In D79868#2034196, @aprantl wrote:

Can you explain why the sunk instruction's location is considered incorrect? I would have naively assumed that for profiling particularly, you would want to retain the original location to ensure that it is counted.

LGTM

I can see that DW_OP_push_object_address would be the norm for a Fortran array with a descriptor, but I wouldn't expect it to be used for all subranges. For example a VLA would need to point to a separate entity containing the upper bound, but that separate entity wouldn't be tied to the object address.

If I take your C input program and compile it to a .o file, I see that we do emit (DW_OP_lit13; DW_OP_lit0; DW_OP_mul; DW_OP_stack_value) for "var2".

LGTM

D35396 is pretty long, so as a public service, summarizing the objections to %T from D35396, having lit always pre-create a %T directory would slow down test runs. Considering %T has been deprecated and work done to remove it from llvm, clang, etc, there are not many tests that use it and it would pretty much be an unnecessary penalty for everyone (except libcxx, who needs to create the directory anyway). (I think I got that right.)

If the value to be printed is only 10 digits wide, I think we do not get 6 leading zeros? Is that how we are formatting 64-bit values in general?

In D78672#2000012, @clayborg wrote:

Thinking from the compiler side: it would be really cool if the compiler could emit all variable sized DWARF attributes at the end of each DIE to maximize how many constant attribute values accesses we could do (if we implement the O(1) access to the value perf optimization in the debug info).

I'm just reading this review for the first time, and my thought was, why is this interacting with implicit-check-not?

I suggest using past tense in the description, because commentary should describe the code as it is (after the patch).
Ex: llvm assembler handled ... it forced ... is_stmt was specified ...

+ llvm-commits

As mentioned above, the commit log should have a short standalone description of the problem/fix. It's fine to cite the bug report as well, but someone doing a "git log" should be able to tell what your patch is about without having to click links or look elsewhere.

It looks like you're not actually interested in the compiled output, but just whether warnings occurred; in that case you'd be better off with -verify -fsyntax-only and // expected-no-diagnostics.

I know it's a debugging/testing option, but it would be good to have a test that shows the two different levels do what you want.

In D76878#1960563, @scott.linder wrote:

Right, I think with so few vendor encodings left there has to be some mechanism for reusing them or opening up the encoding space. Just increasing the vendor extension range will eat into the already dwindling standard encoding space. We also tried really hard to make the entire proposal backwards compatible, and switching to ULEB128 seems like a hard break in compatibility as the 7th bit is already active in DW_OP_'s even in the non-user range. Maybe reserving a single DW_OP_ to say "what follows is a ULEB128 extended-opcode" would be feasible, although it would still have a pretty far reaching impact.

In D77227#1959961, @probinson wrote:

Grepping for \bCHECK(NEXT|DAG|SAME|NOT): in llvm/test and clang/test (that is, a directive missing the hyphen separator) turns up nothing. Are gotchas 4 and 5 really something we should worry about?