LGTM

In D64971#1593796, @vsk wrote:

AFAICT the deref is always needed here: my understanding is that dbg.value has to describe the actual value of a variable, not its address (https://www.llvm.org/docs/LangRef.html#diexpression). I admit that keeping the deref may break the dwarf builder, but istm that indicates a bug in the dwarf builder.

+1 for defaulting to No.
As a follow-up patch, maybe the script could offer to squash the commits for you.

Readability is to some extent in the eye of the reader, so I've restrained myself to one suggestion. Otherwise this review is up to James.

I have been reminded that there's also a desire to make DataExtractor work with 64-bit section sizes. Maybe Cursor should use a 64-bit offset (i.e., size_t not uint32_t), and then migrating from non-Cursor to Cursor APIs will also do the 64-bit transition? We need to bite that bullet at some point. (I kind of expect y'all to say, no way do that later, which is fine; mainly I wanted to refresh that in our collective minds.)

In D64875#1590152, @jyknight wrote:

@probinson I don't understand what you mean -- what's the issue you see with adding this file right now?

This is imposing a requirement on how downstream projects manage their license files. I'm not saying we can't (eventually) do it this way, but I'm quite sure that's not how Sony handles it right now.

In D64544#1589926, @JDevlieghere wrote:

How would you be able to emit a wrong form value for the MD5 hash from assembly? The directive (which I think you came up with?) is just md5 [data]? Deciding which form to emit is hard-coded in MCDwarf.

In D64544#1589853, @dblaikie wrote:

I was rather hoping for test coverage for all the new error messages this change introduced - is that unrealistic/impractical?

Yeah, the line table is especially tricky to hand-craft compared to checking in an object file. I think it technically can still be hand-crafted assembly (no line directives, etc, just a debug_line section with raw byte (etc) directives) - might be plausible & make it clearer what the input is? (checked in assembly, assembled with llvm-mc then run through llvm-dwarfdump to test the parsing)

In D57400#1589807, @greened wrote:

What about downstream users that have added directories in their local forks? Having git suddenly ignore them would be surprising. We are in that situation.

I'm happy, but other people obviously have better eyesight than I do. Give Jonas and Blaikie a day to chime in, I think.

Minor stuff.

This has my blessing as PS4 code owner, but I'd like other eyes on it with respect to how we've gone about it.
+ rnk, rjmccall as the most likely suspects.

If you found the unittests cases because they failed, that's good enough for me.
LGTM

In D64622#1584208, @ikudrin wrote:

I mean, the purposes for the patches are different. This one fixes an existing flaw while your suggestion improves code quality. It is usually better not to intermix such different aims in one patch, no?

In D64622#1583163, @ikudrin wrote:

Right, but maybe that worths another patch?

With ASAN, packs of up to 800 DBG_VALUEs in a row appear (for that file)

This magic number appears in many places; it should have a symbolic name in llvm/include/llvm/BinaryFormat/Dwarf.h, in the LLVMConstants enumeration.

LGTM2

If the change from DW_AT_MIPS_linkage_name to DW_AT_linkage_name made you think the older .o files were MIPS, more likely it is a change in the default DWARF version; older versions of DWARF did not have DW_AT_linkage_name.

In D64231#1578086, @thopre wrote:

In D64231#1573921, @probinson wrote:

I was under the impression that an assertion in the public interface of a library API was not a great idea.

Oops my bad, I forgot about that comment. I think only what's in the FileCheck class should be considered public API. The rest ought to be moved to a different header file and is private. FileCheckNumericVariable is an implementation details IMO.

I guess we can leave the .s file tests alone. Most of them look like they were derived from real -S output and it would be pedantically consistent to update them, but they don't have to be.
@JDevlieghere once you sort out the odr-member-functions the patch is fine with me. I am mildly curious why all three .o files got smaller.

In D63894#1576288, @probinson wrote:

there is no reason to think those paths are tested elsewhere.

In D63894#1574419, @dblaikie wrote:

I would've assumed these conditionals were added by Sony folks for their change in default dialect - that doesn't necessarily mean these tests are needed upstream (the functionality may be tested elsewhere)

Minor stylistic things. Someone who understands passes should look at this.

Minor drive-by stylistic points. Someone who knows what's going on with passes should look at this.

I was under the impression that an assertion in the public interface of a library API was not a great idea.

LGTM

So, the point of this is to be able to remove certain workarounds, rather than advance the language feature set, IIUC.
Note that not all workarounds can be removed if we claim our minimum is the very first release of VS2017; see my r361502, to work around something that all of the bots were happy with.
I'm "once burnt, twice shy" about this, because we're not verifying that our claimed minimum build compilers are actually usable.
What is the minimum version of VS2017 used by any bot? Perhaps we should assert that as the minimum, which then makes it my fault for having a too-old build compiler.

Ping. This is pretty straightforward, the only question is whether we want to preserve these older-dialect tests or rip them out.

LGTM

lit has options?

LGTM

I suspect there are more tests that must or should be updated; grepping for "DW_AT_call_" turns up for example:
llvm/test/DebugInfo/NVPTX/debug-info.ll
llvm/test/DebugInfo/X86/dbg-value-inlined-parameter.ll
llvm/test/tools/dsymutil/odr-member-functions.cpp

+adrian who reviewed the patch that introduced upgrade-pointer-address-space.ll.

In D46628#1559322, @dblaikie wrote:

While all this (the behavior of gmlt) seems unrelated to this patch, really (I agree/understand your description - if you really want to be able to compile things once and link them into debug-ish and non-debug-ish forms, something like the --strip-debug-non-line sounds nice though I can also understand some push-back against it, since it's necessarily going to make broken DWARF, well, except in DWARFv5 where there's an intentional way to support line-table-only debug info (the line table has its own string table, so you can strip /everything/ (including debug_info) except debug_line and debug_line_str, I believe) - perahps that'd be a good way to implement this in a more principled way - work for DWARFv5 only (it wouldn't necessarily need/want to check DWARF version, but be designed to work correctly for DWARFv5 & be weird/bad/problematic before that))

The new API doesn't let you eliminate *all* checks. :-)
Also it introduces some new dependencies as noted inline.

Pick whatever mechanism you like, we should debate it in that patch not here. :-)

Ah, hadn't considered statefulness. But if you layer another class on top of DataExtractor to handle the error flag, it would have to be replicating all the offset-is-valid checks, because of course DataExtractor itself doesn't return errors.

Removing that llvm_unreachable is fine, in that case.
The idea for error handling for DataExtractor sounds reasonable, looks like adding an error flag wouldn't even increase the size.

LGTM but give the West Coast folks a chance to look at it.

I think I am happy with this, leaving the rest up to James.

Looks pretty good, and thanks especially for the error-case tests!
I'll give other folks a chance to chime in if they want to.

We do not precisely match gcc/gas behavior in some more-peculiar cases, but my assertion is that those should not occur "naturally" and so it's okay. For example:

foo:
    nop
    .file 1 "a.c"

This will cause gcc/gas to emit an error to the effect that file number 1 is already defined (implicitly, because of the line-table record for the first instruction). Clang/llvm-mc will not, we'll just emit an odd-looking line table. I can see how to cause clang/llvm-mc to emit this error, but it feels like it would be a hack done just to match gcc's (likely unintentional) error-detection behavior for an ill-formed assembler file.

LGTM.

LGTM

LGTM.

In any event, this is a project policy change and should have an RFC on llvm-dev, not be proposed in a patch.

I think this is a definite improvement in readability, once you fix the two places that seem to have lost a line of commentary.

I see the point; certainly when someone emails a patch, the first response is almost always "can you put this on Phab."

The dwarfdump changes look okay, but the new tests don't exercise those changes. The simplest thing is probably to RUN llvm-dwarfdump in relax-debug-frame.ll and verify the output describes the frame as expected.

Requiring Phabricator raises the barrier to one-off patches from casual contributors, because using Phabricator requires a registration step.
I don't think we should require it until casual users with drive-by patches can contribute easily.

Regarding what to call the @LINE+offset form, I think it's fine to call it "legacy" in the user-facing documentation, but it gets to be a bit much in the internals. I haven't commented every use but you will get the idea in the inline comments.

As I mentioned in PR38449, I plan to look at this starting this week. Even benign situations such as

foo:
.file 1 "a.c"

are tripping over the assertion. I think the correct tactic is not to remove the places that are doing the checks, but make those places do a better job of tidying up anything that had been done in response to the command-line -g in favor of allowing the embedded directives to DTRT.

I am pretty sure all @jhenderson comments have been addressed, and I'm happy, so LGTM.
As I said previously, it may be a while before I get to your next patch.

Ah ha. The ParenGroup referring to the CHECK line as it has been translated for consumption by the underlying regex package... that was not clear.
In which case the terminology is okay but the commentary could be better, see inline comment.

Grammar nits and one longer point, which is:
I understand where the "parenthesis group" term came from, but I think it's not appropriate here. While a CHECK line is implicitly a regex, and variables are a kind of back-reference, the syntax for variable references (use or def) is not parentheses and the content is not a "group" in any real sense, and so "parenthesis group" is an un-obvious and confusing term.
Because FileCheck uses square brackets, I propose that the least disruptive change at this point would be to call them "bracket expressions." The "bracket" part is obvious, and "expression" because (a) definitions will have a regex, and (b) numeric variable references are implicitly expressions. (Okay, still a bit of a stretch, but less so that "parenthesis group" IMO.)
WDYT?

Is the compiler missing a check that these parameters are immediates?

LGTM

In D62634#1527575, @dblaikie wrote:

This is intentional behavior in clang (controllable under the -f[no-]split-dwarf-inlining flag, and modified by the -f[no-]debug-info-for-profiling flag).

This extra debug info is used for online symbolication (in the absence of .dwo files) - such as for the sanitizers (accurate symbolication is necessary for correctness in msan, due to msan's necessary use of blacklisting to avoid certain false positives) and some forms of sample based profiling collection.

In the default mode, clang includes, as you noted, just the subprograms that have inlined subroutines in them in this split-dwarf-inlining data.
In -fdebug-info-for-profiling all subprograms are described in the skeleton CU with some minimal attributes (they don't need parameters, local variables/scopes, etc) necessary to do certain profile things I don't know lots about.

Well, the results are at least defined now. Please make sure the commit log mentions "e.g. fold-sext-trunc.ll".
LGTM

LGTM, although I commonly see llvm::make_unique to document that we are specifically not using the std:: one. On occasion it is actually ambiguous.

Looks like the right solution, but have you actually tried it with an empty default triple?

Drive-by: For the "dead code" did you check whether gcc emits DW_AT_start_scope? LLDB should support more than just what Clang emits.

Apart from missing 'override' keywords it looks pretty straightforward. LGTM.