In D62118#1510389, @dblaikie wrote:

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

The helper is passed as a template parameter to a class ctor, and that instantiated class calls the helper from operator(), so I suppose maybe enough indirection through lambdas....
but this seems kind of involved for getting my builds to work.

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

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.

LGTM

That's a great idea, especially the "string variable" and "string substitution". I was not pleased with the name either but the only thing I thought of was regex variable which is plain wrong, I completely missed the obvious.

Naming is hard, I clearly didn't think of it right away either.

There's still some inconsistency in the terminology, as clearly evidenced by the name and description of the IsNumExpr member of FileCheckPatternSubstitution. That class used to be where we'd find variables (now called pattern variables); but it has had numeric expressions crammed into it, except the comment wants to call it a substitution, and so we have "pattern" and "substitution" and "expression" all competing for clarity.

Minor stuff. This solution is surprisingly simple, the main question being (and I don't have an answer) whether increasing the size of PresumedLoc is okay.

It still bothers me that MCDwarf has to know exactly when to override the target's decision to do relaxation.

It sounds like inlining a function with a loop has a bug with respect to how the loop metadata is handled, and your patch merely tripped over that. Would it be reasonable to fix the inlining-loop-metadata bug separately first? And then the original patch is likely to Just Work?
Fixing one bug at a time is more in line with project practices.

LGTM once you rephrase the comment in the test.

LGTM after you add the full-stop noted below.

Starting set of comments on this one. Haven't looked at everything yet.

LGTM

@stella.stamenova fixed this using system-windows instead.

In D61611#1496865, @stella.stamenova wrote:

In D61611#1496838, @probinson wrote:

@stella.stamenova I'm not familiar with any lit feature that gives a special meaning to the prefix "no". The opposite of "REQUIRES: windows" is not "REQUIRES: nowindows" but "UNSUPPORTED: windows" AFAIK.
This part of the discussion should probably be taken to llvm-dev, though.

FTR I don't see that lldb's lit.cfg.py sets any features based on host OS, so even "UNSUPPORTED: windows" probably does not work currently.

@probinson LLDB's lit configuration derives from the lit configuration in LLVM. The most important file you are looking for is:

llvm\utils\lit\lit\llvm\config.py

You can see there that we add various platform to the list of available features including system-windows, so XFAIL: system-windows, UNSUPPORTED: system-windows, etc. all work.

This is also where binary_features are set, such as 'zlib/nozlib', 'asan/not_asan' etc. The prefix varies for historical reasons, but the paradigm has existed for a long time. If we wanted to support nosystem-windows, we would add it here.

@stella.stamenova I'm not familiar with any lit feature that gives a special meaning to the prefix "no". The opposite of "REQUIRES: windows" is not "REQUIRES: nowindows" but "UNSUPPORTED: windows" AFAIK.
This part of the discussion should probably be taken to llvm-dev, though.

In D61611#1496580, @sgraenitz wrote:

@stella.stamenova Can you have a look at the lit test please? It works on macOS and Linux, but I didn't test Windows. Should I add something like # REQUIRES: nowindows or is it fine like this?

I am pretty happy with this now, maybe give the other people who have been participating a day or so to chime in again.

Still good.

One or two last nits and LGTM.

I wonder if this should be an MIR test? Fewer moving parts.

Sorry, I should have been more thorough about this previously: My understanding is that if a function description mentions any parameters with \p, then it needs to mention all of them.
For some of the one-liner descriptions, if you prefer to just remove the \p that's okay with me. You've been suffering a lot through this process and I don't want to create more work than we really need to!

Fix the one typo and LGTM.

The point is not that a certain set of moves comes out, but that the *same* set of moves comes out regardless of debug info.
So, the test wants to have two copies of the same function, one with debug instructions and one without; and both functions should produce the same sequence of moves.

Aha. The new versions add the word "pattern" to some of the diagnostics, in pattern-defines-diagnostics.txt. That would be the functional change belonging to D60385, which means that test should not pass with an unmodified FileCheck? Please verify.

In D60385#1494062, @thopre wrote:

In D60385#1493635, @probinson wrote:

I still can't tell what the functional difference is, in pattern-defines.txt. Everything else looks fine.

There is none, pattern-defines + pattern-defines-diagnostics = old defines.txt + some new comments, as per James' comments.

I still can't tell what the functional difference is, in pattern-defines.txt. Everything else looks fine.

Thanks for the reference, that helps some.
However I still think this isn't the right way to go about fixing the problem. This started because RISC-V decided not to resolve certain kinds of expressions, and then it turns out that applies to too many expressions. Somebody suggested that the asm backend decision should be more refined, based on symbol section perhaps, and that feels like a more principled way to go about it.

+1 to Bjorn's comment.
The DWARF spec says address class is also allowed on a "subroutine or subroutine type" do we allow generating code into nonzero address spaces?

Nearly there! One final grammar nit and some test comments.

I don't claim to be an MCExpr expert, but I think this should not be necessary. There are other situations where symbol differences are computed and emitted as constants, for example when DW_AT_high_pc is a length rather than an address, and they clearly don't require this tactic.

Checked-in files should not have CRLF endings, in general (there are a very few exceptions, and these aren't among them).
If the checked-in files have LF endings but your tool checks them out with CRLF then that is a problem with your config, or with the tool.
Adding dos2unix to the RUN lines is the wrong fix, either way.

Thanks! Some small things, and LGTM once you've fixed those.

Haven't looked at the tests yet.

LGTM

Another round of commentary nits, plus a couple more substantive remarks. If you accept them all, LGTM.

FYI, we had to disable clangd entirely after this patch, getting a weird problem with lit that we haven't figured out yet.
In case anybody else has seen something like this, and knows what's going on, please let us know.

  Running all regression tests
  llvm-lit.py: C:/j/w/opensource/opensource_to_sce_merge__2/o/llvm\utils\lit\lit\llvm\config.py:336: fatal: couldn't find 'clang' program, try setting CLANG in your environment
C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 2. [C:\j\w\opensource\opensource_to_sce_merge__2\build\check-all.vcxproj]

First pass looking at docs and commentary.

In D60283#1480546, @aganea wrote:

Thanks Paul, your solution is even better. I'll apply rL333311 locally - if everything's fine, do you mind if I re-land it again?

I had tried to do this in r333311 but some bots complained, so I reverted it and then didn't follow through. Thanks for doing this!
When I tried it, I took advantage of existing tracking of line directives at the file level, so there shouldn't be a need to add a Line flag to PresumedLoc?
What I did was something like this, in computeChecksum():

const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(FID, &Invalid);
if (Invalid || !Entry.isFile() || Entry.getFile().hasLineDirectives())
  return None;

I'm okay with this, but give @aprantl a chance to confirm.

I'm okay with the PS4-specific bits.

LGTM after the comment on the parseVariable loop is addressed.

LGTM

In D60831#1472621, @jmellorcrummey wrote:

This will be my last comment on the topic and then everyone can get back to work :-). I agree with the LLVM IR requirement that one not to lie to application developers; lies are worse than nothing. However, I don't feel that you have adequately explained why attributing to any of the available (file, line) mappings for an instruction that has been merged is incorrect and not 100% reliable. Just because we can't include all contributing mappings for a merged instruction doesn't make any one unreliable; I see attributing to any one of a set of mappings as 100% correct, even though it is only partial information.

LGTM. The previous diagnostic would be useful only to compiler developers.

In D58033#1470260, @djtodoro wrote:

@probinson @aprantl Thanks a lot for your comments!

Let's clarify some things. I'm sorry about the confusion.

Initial patch for the functionality can be restricted by this option (like we pushed here), since LLDB doesn't read this type of debug info (yet).
The plan is, when LLDB has all necessary info and we are sure that everything works fine during using this info in debugging sessions, we can turn it to ON by default.

Does that make sense ? :)

I guess I'm not clear what your final goal is for the option. Keep it, even though GCC doesn't have one like it? Eliminate it? Please clearly state what you intend to have in the end, and what you might have in the short term in case that is different.

In D60382#1466257, @thopre wrote:

Therefore, if we want to remove the diagnostic from that function we need to remove the ability to define a numeric variable from a numeric expression (which allows stuff like -D#NUMVAR1=10 -D#NUMVAR2=NUMVAR1+4. How useful do you think this ability is?