Ping.

FastISel is normally used only at -O0, I wouldn't expect any parameters to be optimized out at -O0.
The test is running llc with default optimization, which is -O2, and forcing fast-isel; this is not a usual combination and I wouldn't expect us to spend any effort making sure debug info works well with that combination.

I wasn't sure whether setUp() was supposed to be SetUp() or not. Good to have that confusion go away!

FTR, this looks okay from the PS4 owner's perspective, although IIRC @wristow did our original local patches.

Addressed comments.

Remove ifdefs, tweak descriptions/comments.

In D99250#2651953, @probinson wrote:

In D99250#2651204, @dblaikie wrote:

Does anyone else have a DWARFv3 consumer they care about? (@aprantl and @probinson)

Does anyone have a DWARF consumer that changes significantly based on the language version (I guess lldb (@aprantl can you confirm?)? I doubt gdb does (yeah, it just treats all C++ versions the same)? how about Sony?)

I believe Sony is only using v4/v5 at this point. Regarding dialect controlling debugger behavior, I think it's unlikely, as distinguishing C++ dialects is new in v5, its adoption is pretty recent (at least for us), and we'd want behavior to be consistent across v4/v5 as much as possible. I'll verify that with our debugger guys.

Problem has gone away, test assertions are now executed.

In D99250#2651394, @jsji wrote:

The list is growing, but sure, we will post a thread in llvm-dev about what we met so far.
Two big one would be that DBX not supporting string section(DW_FORM_strp) and column-info in line no table.

I think there are degrees of compatibility with regard to DWARF, which is inherently supposed to be extensible if the extension can be safely ignored by a consumer. This is the "permissive" rule in the standard.

In D99250#2649507, @Esme wrote:

Thx! @aprantl The motivation of the patch came from the crash of tag name mismatching when using DBX under AIX. And modifying the debugger doesn't seem to make sense?

Please add words to the effect of: When re-landing a reverted patch, the commit message should be updated to indicate the problem that was addressed (bot failure, cite the PR, whatever).

Welcome to the ranks!

+gribozavr who has been in this file most recently

In D97411#2598625, @akhuang wrote:

I started looking into some diffs of debug info in libc++ tests, but it's pretty hard to tell what's different - as far as I can see, there are just a bunch of __hash_value_types and __value_types.

As far as I know, it does. However, before CHECK-LABEL, a CHECK-NOT range is not the same as a CHECK range:

The strange part to me is that a preceding directive's search range ends at the end of the CHECK-LABEL match. It seems like it ought to end at the start of the CHECK-LABEL match.

In D97446#2587996, @MaskRay wrote:

In D97446#2587806, @probinson wrote:

GNU ld has a rule "start_/stop_ references from a live input section retain the associated C identifier name sections", which LLD may change in the future (D96914).

The phrasing "may change" implies LLD could eliminate the rule; in fact D96914 will only add a way to opt-out of the rule. You can't eliminate the rule entirely without breaking a lot of useful cases.

Reworded this sentence. D96914 does add a way to opt-out of the rule. In the future (when toolchains are more mature) we may try. My internal tests and Linux kernel tests are try to state that we may not have many dependent cases.

GNU ld has a rule "start_/stop_ references from a live input section retain the associated C identifier name sections",

which LLD may change in the future (D96914).

Aha; attribute used *by itself* is not sufficient to preserve sections in the output. But the __start_/__stop_ symbols implicitly create a reference to each of the named sections, and that implicit reference can preserve them in the output (assuming gc roots etc).
So, the idea is that attribute retain can be used *instead* of the __start_/__stop_ symbols, to preserve sections in the output (with the advantage that it will work even for sections that do not have a C-identifier name).

For ELF, used does not retain the entity, regardless of this patch.

A few typos. Nice to have these!

the driver had a redundant pass-through of the option

I don't claim to be very familiar with this area but "round-trip" and "test" would make me think those bits should be in a lit test or unittest. As it is, it's not obvious what is functional and what is testing here.
Possibly I am misunderstanding something fundamental about how these options work?

I've re-read a fair amount of the previous llvm-dev thread.
One goal of doing a patch like this was to collect some performance data; do we have that? In particular, performance data comparing link times with the option default-on versus default-off, so we can determine whether the difference is small enough that we should just do this processing unconditionally.

Modified tests as recommended by Quentin on llvm-dev; I hope I got it right. Wasn't clear if the intent was to modify *all* tests in the file the same way (adding B.setInstr(foo)).

Seems reasonable, provided you correct the language to be about building a cross-compiler rather than actually cross-compiling.

Ping

I asked about this on llvm-dev but it seemed worth reporting here as well.
Some of the differences between the expected output (per the FileCheck directives) and the actual output appear to be little more than reordering of the instructions. However, oddly, in some cases the use of a value might precede its definition. For example, in NarrowSEXTINREG, the test is looking for this:

CHECK: [[T0:%[0-9]+]]:_(s16) = G_TRUNC
CHECK: [[T1:%[0-9]+]]:_(s10) = G_TRUNC [[T0]]:_s(16)
CHECK: [[T2:%[0-9]+]]:_(s10) = G_SEXT_INREG [[T1]]:_, 8
CHECK: [[T3:%[0-9]+]]:_(s16) = G_SEXT [[T2]]:_(s10)

But, what I see is this:

%5:_(s16) = G_SEXT %7:_(s10)
%4:_(s16) = G_TRUNC %0:_(s64)
%7:_(s10) = G_SEXT_INREG %6:_, 8
%6:_(s10) = G_TRUNC %4:_(s16)

So the SEXT uses the result of SEXT_INREG, which uses the result of the second TRUNC; these are all forward references. That doesn't seem correct; uses before defs? If there's a problem with instruction ordering, fixing that seems like its own issue, and would probably make many of the failures I'm seeing just go away. (Global ISel is very much not my area, so I'd appreciate it if someone else could figure out the ordering question.)

In D82363#2506919, @StephenTozer wrote:

W.r.t the replacement of DBG_VALUE with DBG_VALUE_LIST, the current plan is to make that all happen in a patch following this stack. The patch itself should mostly be deletion of old code and simplifying where possible, renaming DBG_VALUE_LIST to DBG_VALUE, adding an extra operand to the new DBG_VALUE to represent directness, adding several tests, and updating existing tests as appropriate.

It's possible that the InstantiationGroups result is not supposed to be empty; in which case, something deeper is wrong, and I can abandon this patch and file a bug. Let me know.

I'd expect the pace of preparing the release to be basically the same regardless of start date, so the milestones would make more sense as start date plus N weeks.
If you want wiggle room for the start date, that's fine, anything that expresses "late January" and "late July" will work. I thought it was going to be more definitive, what with specific week numbers and all.

I think "Week number" is too ambiguous to be a guide. If January starts on the last day of the week, does that still count as week#1? What day does the week start on, anyway--much of the world starts the week on Sunday, much of the world starts the week on Monday.
"Fourth Tuesday in January/July" is unambiguous and makes everything easier to plan. Using "Start + N weeks" for the rest of the target dates is fine.

Catching up on this, sorry...

In D91734#2482318, @dblaikie wrote:

But, yes, we could possibly do better with more strategic is_stmt, but that's a big/complex piece of work to tackle.

In D91734#2480930, @dblaikie wrote:

I haven't looked closely, but I take it this one test modification seems reasonable to you? I guess we're stepping into some subexpressions on a function call that we previously didn't? (they didn't have any instructions attributed to them until now, but it's not unreasonable that they have them attributed - for materializing constants to pass to a function call when the function call/constants are split over multiple lines, etc)

This version of the patch avoids the weirdness I was seeing with prolog instructions in certain cases.

Change how PHIs are handled; if the operand has a debug location, use it, otherwise don't set a debug location. Then, flushLocalValueMap() will look at the first local-value instruction; if it doesn't already have a debug location, we copy the location from the first instruction after the local-value instructions (which should have the debug location of the original IR instruction).

Thanks Duncan! I (or someone) will play around with that and see what we need to do. May take a little while, as we're about to freeze a release and then go on break for two weeks, but good to know there's a straightforward path.

One thing this patch does, is make decisions about default behavior static. Meaning, the option behavior cannot depend on other options; specifically, it can't be based on the triple, which allows target-specific customization. PS4 certainly has cases where our defaults are different from the usual ones, and I'd kind of think that was true for other targets as well.

I've run this through our copy of the GDB suite (8.3, not sure how old that is). There are 10 differences, with and without the patch.

This is still showing as approved; I'm going to try "Request Review" to see if that helps.

Rebase and combine cf1cc774d and dc35368c, plus revise handling constants materialized due to PHIs.
I plan to do some build-time and object size measurements, similar to the first time around.
I plan to run this against the gdb test suite, or at least the local copy we have here at Sony, and post my findings.

Reopening, will upload a new diff in a moment that modifies how PHIs are handled. I intend to run the gdb suite on this, will post findings when I have them.

In D91734#2432988, @dblaikie wrote:

Yeah, it boils down to something like this:

void f1(const char*, const char*) { }
int main() {
  char prog[1];

  f1(prog,
     prog);
}

Without the patch, you step to the 'f1' line, and then step into or over the function call (step or next).
But with these patches applied - you step to the 'f1(' line, then the 'prog);' line, then back to the 'f1(' line, then into/over the function call.

Again, could be acceptable - if those arguments had specific non-trivial code in them (like other function calls) you'd certainly get that step forward/backward behavior - but it's notewhorthy that this is change would make more cases like that & it'd be good to understand why/if that's a good thing overall.

And with these changes together, breaking on the function breaks on line 5 (presumably because this is creating the constant used by the second '&&' which is on line 5) instead of line 3. Not the worst thing, but I imagine given Sony's interest in less "jumpy" line tables, this might not be super desirable.

It's breaking on an xorl %eax,%eax which is produced by the PHI at the end of the conditional expression, which now has the source location of the operator at the top of the expression tree, which is the second && operator, yeah. Not the best. (FTR, the jumpiness complaints we get are usually more egregious, hopping between different source statements somewhat arbitrarily; not sure anyone would complain too loudly about hopping around within an expression. We see less of that in any case, because we suppress column info, but still.)

See D92606 for a front-end patch to improve locations in the IR.
That, plus reapplying this patch, should help out GDB. I haven't had a chance to run the suite myself with both patches applied and I'm basically off tomorrow, so if @dblaikie doesn't have a chance, I'll do that on Monday.