LGTM with some minor comments. I'd feel more confident if someone else (/ @jmorse ) took a quick look too, but given this is NFC and conceptually straightforward, I'm happy to approve it.

LGTM

The test LGTM - I'll leave it to @jmorse or @fhahn to accept when a consensus is reached on whether a bounded version of sizeWithoutDebug should be added or not.

The code changes look good to me, so just waiting for the test before approving now.

Should add a .mir test

In D126767#3562844, @jryans wrote:

Thanks for taking a look @Orlando!

Unlike most historical LLVM docs, this file is using Markdown, which ends up rendered together with the reStructuredText docs by Sphinx. Markdown is recommended for new docs.

With Markdown in mind, please take another look when you have time. 🙂

I've added an inline suggestion but I'm happy with or without that change, LGTM.

LGTM

LGTM!

Hi @jryans, thanks for this update. I'm not particularly familiar with reStructuredText formatting so I've just been looking at an existing doc (https://github.com/llvm/llvm-project/blob/main/llvm/docs/LangRef.rst) to guide my review.

Just to confirm, does this comment:

Remove unused "zero range" test.

It looks like the clang front end emits a size field for reference types. e.g.

LGTM with some nits addressed, thanks! I've hit accepted but please leave this up for a day or so in case anyone else has any further comments.

The latest change makes sense, LGTM

Makes sense, LGTM. I've added two tiny suggestions inline but I don't mind if you leave the patch as it is.

LGTM. It's great to have the comprehensive test spelling out the expectations for all the combinations of fragment / deref / indirect.

LGTM

In D123599#3445402, @jmorse wrote:

/me squints -- looks like the test doesn't cover the case of expressions with fragments, which I'll revise in later. (The patch has a case for that).

I wonder if the test IR could be simplified. Would it not be enough to have a function with a single dbg.value and nothing else?

FWIW the change LGTM mechanically and I think it sounds reasonable (/good).

Hi @yln, the test you added llvm/test/Transforms/LowerGlobalDestructors/lower-global-dtors.ll is failing on some bots, e.g. https://lab.llvm.org/buildbot/#/builders/139/builds/18527 (builder llvm-clang-x86_64-sie-ubuntu-fast). Please can you take a look?

Makes sense to restore NFC-ness of the patch, LGTM with that change.

Please wait for the other reviewers to be happy, but this LGTM with some nits addressed.

In D120168#3363746, @chrisjackson wrote:

Reinstate the check for dbg.value that LSR has changed from single location-op to multi location-op. When LSR makes this change, the single location argument is changed to a DIArglist. This patch now replaces the DIArglist with a single 'undef'. This will prevent the generation of invalid dbg.value that were causing the crash as reported by @Orlando .

The change SGTM but I found a reproducer that causes clang to crash when this patch is applied:

In D120464#3363274, @jansvoboda11 wrote:

In D120464#3363258, @Orlando wrote:

Hi, the test add-remove-irrelevant-module-map.m starts failing with this commit on the llvm-clang-x86_64-sie-win builder: https://lab.llvm.org/buildbot/#/builders/216/builds/929. Please can you take a look when you get the chance?

Hi, it's already fixed in https://lab.llvm.org/buildbot/#/builders/216/builds/932.

Hi, the test add-remove-irrelevant-module-map.m starts failing with this commit on the llvm-clang-x86_64-sie-win builder: https://lab.llvm.org/buildbot/#/builders/216/builds/929. Please can you take a look when you get the chance?

I can clarify the comment. It's my understanding that non-empty DIExpressions must have a DW_OP_stack_value terminator. The expressions generated by the SCEVDbgValueBuilder objects do not have these terminators (as the expressions may be inserted into other expressions). If the pre-LSR expression was empty, once the complete salvaging expression has been generated, then the terminator must be appended.

This should improve the readability of the output nicely. LGTM.

LGTM

I don't think there are "rules" about tagging patches as NFC (just put NFC somewhere in the title). It's definitely helpful to be able to see at a glance though - two examples: if an NFC patch is causing changes in output there's a bug, and if a patch is NFC it probably doesn't need any tests.

In D118774#3349045, @dblaikie wrote:

Looks good on my side - might be worth checking with @Orlando too before submitting.

LGTM

Can this be labelled NFC or is there a functional change here that I've missed?

Thanks @gkistanova. I don't think I have commit access to llvm-zorg. Please could you land this for me when you get a spare minute?

LGTM with query; More restructuring to avoid more indentation would be nice, but here we are ._.

+ addressed inline comment

+ Improve comment in test
+ Fix description

+ Rebase (fix conflicts with D118468)

LGTM. Converting badly-formed variable locations into "no location" rather than crashing the compiler SGTM. Is it worth fixing the defect in the bug report too in a separate patch?

+ Make gdb-version-check more robust by checking version string length

+ Catch-em-all when trying to run gdb
+ Print a message when the XFAIL condition is detected.
+ Fix stderr -> sys.stderr in a print call.

We've now got a native Linux builder in staging where you can see these tests failing: https://lab.llvm.org/staging/#/builders/205/builds/91

We've now got a native Linux builder in staging where you can see these tests failing: https://lab.llvm.org/staging/#/builders/205/builds/91

In D118460#3284107, @Orlando wrote:

Would it be worth caching getBlocksForScope results? If I'm reading this right it looks like it's called a few times with what should be the same inputs and outputs in a few places in the pass?

Nice. I have a few questions / nits but I'm a fan of the change.

Thanks for taking a look @probinson. Adding Jeremy as reviewer.

+ llvm-commits

LGTM

LGTM

Nice! I have one question inline but this SGTM.

Once @dblaikie's concerns are addressed, this LGTM.

The clang-format bot isn't happy and I have one nit inline, otherwise LGTM.

nit in commit message:

This patch releases some memory from InstrRefBasedLDV earlier that it would otherwise. The underlying problem is:

"earlier that it would" -> "earlier than it would"

Folded nit-fixes into the commit, thanks both.

LGTM

That clears up a lot of my confusion.

Sorry it's taken me a while to get back to you on this - thanks for all that info. Here is a summary of the situation as far as I understand it:

+ Fix comment in test: -g -> -gdwarf-5

Thanks for taking a look @probinson.

(Still LGTM)

Hrm :/ I'm not super enthusiastic about either direction. I'm starting to wonder whether this general direction is viable - that some DWARF consumers might not find types that are in type units but aren't referenced from the CUs at all.

Do you have a measure of the debug info size improvement of this direction? If it doesn't end up counting for much, maybe it's not worth trying to create this possibly novel debug info?

Please can you update Commands.md too? LGTM with nit, thanks.

Thanks very much for the reproducer and revert @dblaikie.

There's a mistake in the commit message, I meant "Example of failing bot: https://lab.llvm.org/buildbot/#/builders/123/builds/7772".

I appear to have broken a bot here https://lab.llvm.org/buildbot/#/builders/105/builds/18850, looking now.