In D87302#2810683, @AndrewLitteken wrote:

I think that's certainly possible. If there was a way in the debug info to say that a specific section of IR is from Section A of the program if called from location A, Section B if called from location B etc that would solve the problem. I don't know enough about DebugInfo to really say how difficult that would be though.

In D86813#2797746, @aprantl wrote:

Yes! Bugpoint can already reduce metadata — I'm wondering if this could maybe a mode of bugpoint in the future?

Ping and refresh of this patch; I've replaced a multimap with a sorted vector, used a class to store information about seen DBG_PHIs, and twiddled some documentation.

@jmorse @aprantl Sorry for the divergence, but regarding this tests reduction in terms of DI Metadata -- do you think that we can automate this process? I've written a small write-up/proposal for a utility for that purpose: https://github.com/djolertrk/llvm-metadataburn -- please let me know what you think.

Address feedback, delete various un-needed MIR bits and pieces, un-necessary DILocations.

LGTM plus nit.

LGTM

For the record, this LGTM too. Picking a better cut-off point is probably something we can experiment with in the future.

ping here; I can see the tests here can be improved by removing attributes and a large amount of un-necessary MIR headers, otherwise IMO it's ready to go

NB: I'll probably drop this in sometimes soon, on the back of @djtodoro 's LGTM earlier

This makes sense to me: the range extension mechanism puts in DBG_VALUEs at points where the live value switches from one register to another, something that LiveDebugValues can't know later on, and often gets wrong. Instruction spans with the "wrong" scope in the middle of a block will prematurely terminate ranges -- or in the case of the prologue, it won't cover early instructions. I think there's also potential for an argument that's moved more than once in the prologue to be dropped, not completely certain.

In D91722#2774557, @sbc100 wrote:

After the third re-land of this patch we are seeing a crash in the emscripten compiler: https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket.appspot.com/8846604336499720256/+/steps/Emscripten_testsuite__upstream__other_/0/stdout

Ping and update for @djtodoro 's comments -- some minor editing and deleting of un-necessary parts of tests.

LGTM given that I've seen a reproducer where this causes an assertion failure -- it's clearly best to avoid the assertion failure. We should re-visit this in the future as there are quite legitimate scenarios where this would be important, for example using optimised LLVM-IR with llc -O0.

LGTM

LGTM. While this is a breaking change, AFAIUI the body of Dexter tests "out there" is small. If this is a serious issue for someone, we can revert and come up with a better plan.

LGTM

LGTM with D102841

LGTM

Minor inline nits that can be skipped, plus a question about relying on DenseMap ordering.

This works fine with our workflow at least, so a soft LGTM from me. I'd suggest waiting for a couple of days for more feedback though.

[Digging back through my inbox,]

We need a refactor here to change all places that call addBlock(, location_attr, ) to use DIEDwarfExpression

Adjust wording from review; I've added a 'tikzrender.tex' latex header and instructions on how to produce a png from a .tikz diagram. Alas, I'm more familiar with tikz than graphviz.

Hi @shchenz, is it feasible to use DIExpression::isImplicit at a higher level to filter out these kinds of expressions? I believe it's possible to parse a DIExpression before it's lowered to a DWARF expression, and there are places where we rely on the isImplicit method for correctness already. Also paging @StephenTozer who's touched this area recently.

In the code you've seen, does the debug info all or mostly consist of DBG_VALUE_LISTs now, or is it still mostly DBG_VALUE, or a mix?
I'm just trying to get a sense for how much of a degradation in debug info quality we have now.

NB: do you need this and D100270 to be pushed for you?

In D101523#2750300, @StephenTozer wrote:

I'll open a bugzilla ticket about the issue, but since it exists before this patch I think the fix can come later.

Hmmmmm. What if we have:

NB: a pre-render of the text and diagrams can be found here:

This LGTM modulo one inline comment, plus: the test is in the "Generic" directory, but it is not fully generic. It depends on the armv6 triple being supported, so things like the powerpc build bots will likely trip on it. You should add an appropriate REQUIRES line.

I'm not familiar with WebAssembly, but thought I'd describe how this would works in other backends -- it might inspire you to take a different route. Sorry if it doesn't apply to WebAssembly.

Looks mechanically correct, test wanted seeing how this was missed before.

Looks good as a starting place -- I took this for test drive, see inline comments. On a clang-3.4 build, the "PC ranges covered" statistic ticks down from 66% to 65%, however I believe that's due to rounding. Using llvm-locstats, roughly six thousand variables move out of the 100% covered bucket, or about 0.2% of all variables in clang-3.4. 0% covered goes up by a negligible amount. In my opinion: this is a reasonable sacrifice to make in the name of accuracy, and some of those dropped locations will be incorrect. Plus, there's a longer term plan for not needing this.

Pushed up; you might get buildbot emails about tests failing, as commits are tested in batches. I'll keep an eye on those.

Can you help me to commit the changes as well. I don't have commit access. Thanks, @jmorse

LGTM

LGTM

Looking at the output of the test, it looks like the dbg.value(undef) is in the scalar tail of the loop, rather than vector.body, right? This is slightly cross purposes to what we want, I think: the induction variable value is unavailable during the vectorized portion of the loop and should be undef, but the scalar part isn't optimised, and so the value is still available.

LGTM

LGTM with a nit

LGTM, I enjoy the minimal extension of a test to improve coverage too.

Right you are -- another thing I didn't clock was that this test was running all of the LLVM passes (generating many instances of the coroutine function). Latest revision disables those passes so that there are only DILocalVariables for the un-coro-split function. I've confirmed this fails before the fix landed and passes afterwards.

Orlando wrote:

the linked document also says "a transformation should also preserve the debug location of an instruction that is moved between basic blocks, if the destination block already contains an instruction with an identical debug location."

Digging deeper, it looks like this was discussed in the patch that introduces the wording to the document (D93662). cc @fhahn @vsk @jmorse. wdyt should happen here?

LGTM

This is a good find; style nit and test comments inline.

I think this is good to go, with some revisions to the test.

NB, while I flagged that CHECK-NEXT added too much ordering, I forgot that CHECK still requires some ordering. It causes us some difficulties downstream; there's a follow-up patch here D100298 for consideration.

LGTM with latest memory jiggery pokery.

LGTM with a nit, and modulo @markus 's comment.

I ended up taking this for a test drive, and found some difficulty when using the LLDB bindings -- _add_conditional_breakpoint ends up being passed a PurePath object for the file_, which is then passed to the LLDB bindings, which croak. This is easily fixed by calling str(file_) to convert the PurePath object to a string, but this might affect other debugger drivers & need some assertions adding.

LGTM, with a couple of nits, plus Orlandos comments should be addressed.

Uh, I just figured out how mir tests work.

Thanks for the reproducer; I think I see the same as you, when LiveDebugValues runs:

The test now passes without the rest of the patch applied; as mentioned above, it needs to affirmatively identify the contents of retainedNodes to guarantee detection of extra entries. Alternately, use CHECK-NOT or --implicit-check-not to confirm the absence of the extra DILocalVariables.

Happily we can consult https://llvm.org/docs/HowToUpdateDebugInfo.html -- the example that you've given looks like's a good test for the "positive" case, where we should keep the source location attached to the compare instruction. The comparison doesn't get moved between blocks, so we match "When to preserve an instruction location".

I like the direction of the SelectionDAG change; reducing duplication of information is a worthy goal. My knee-jerk reaction is that we need a test for it, but I imagine it's hard to hit something that deep into DAG Combining. If you've got a reproducer to hand, it'd be good to add that as a test, if not maybe we can skip it. IMO it's also worth breaking this off from the LSR modification.

Sounds good overall, with a comment inline about the test for isSignedConstant. IMO, there should be some test coverage of expressions that shouldn't be recognised as a signed constant too.

FTR, the patch LGTM. I haven't really been following the development of debugify in detail so I have no feeling for whether this is good from a high level; if you're confident the mailing list discussion finished with this being the right direction, I'd say this is ready to land.

There isn't a lot of difference to how debug-info is updated, as far as I can see, but the summary says a fault is resolved. Is this patch just masking the debug-info issue by doing more propagation? (I might have missed something in the implementation).

Hmmmm, do you have a reduced reproducer in llvm-ir that could go in a bug report? There are a number of things that could be going on here, and we can't be sure which without an example.

When looping over the debug users of instructions and replacing the dbg.values operands, you should now use the "replaceVariableLocationOp" helper instead -- this avoids re-creating a dbg.value intrinsic, and should handle variadic variable locations (the patches for which are 95% landed) seamlessly.

Drive-by review: some minor nits commented on, looks very reasonable overall.

To my mind, the two attribute names, _conditional_bp_ranges and _conditional_bps are quite similar -- but AFAICT the former is a on-initialization record of where conditional bps should be placed, while the latter is a "live" record of conditional bp handles, is that right? IMO, better to have an indication of this distinction in the name, may I suggest _conditional_bp_handles?

Passes on Windows just fine, thanks for the follow up!

It turns out there's a plain old windows-has-different-slashes problem going on. In the Test object __init__ method, the call to os.sep.join in this patch produces the string "subdir\ccc.txt", which won't match "subdir/ccc.txt" that you load from the the .lit_test_times.txt file. That then leads to no time being loaded for ccc.txt, and it gets mis-ordered.

I'm experiencing reorder.py failing intermittently on a Windows system -- it seems to pick up the .lit_test_times.txt produced in the build directory for the Inputs tests after the first run, and use that for ordering. And seeing how that just contains noise, the Inputs tests are then run in the wrong order. Is this supposed to be accounted for already?

In D91722#2617949, @StephenTozer wrote:

Modified salvageDebugInfoForDbgValues to only produce DIArgLists for dbg.values, and not dbg.addr or dbg.declare. This is necessary for the time being, as the current implementation of variadic debug values only works with DW_OP_stack_value for reasons previously discussed on the mailing list and in the root patch of this stack. In the next patch to follow, which will make the current DBG_VALUE_LIST into the default debug value instruction, the DIExpression representation will be changed to support non-stack_value variadic debug values, and this change can be reverted.

In principle this looks good, but as @djtodoro points out it needs a test.

In D86377#2611282, @kubamracek wrote:

I've uploaded a new version of the patch (just moving the definition of HAS_48_BIT_ADDRESS_SPACE outside of the SANITIZER_GO branch). @dyung @jmorse could I ask you to check this patch whether it builds correctly on x86_64/linux? Thanks!

LGTM, but pls2leave it 24 hours or so in case @rnk chimes in.