Wow :-)

I'm expecting this to cause all sorts of non-clang frontends to trigger this assertion, since we don't require types to have sizes. In fact, I have an open bug for the Swift frontend to try removing the static sizes from types, since LLDB prefers the dynamic runtime size anyway.
If we want to make this a requirement, we should probably implement it as a verifier check instead of an assertion. But I'm not sure if we want to require this for all frontends.

Thank you!

This looks like a good idea — people will probably have some options on the names and descriptions of the statistics :-)

I see

Is this causing real problems? Usually we emit newer attributes even for older DWARF versions because most consumers just ignore unknown attributes.

Could you add test that showcases the effect?

I think this looks mostly good.

Assuming that it works, this seems fine.

Ignore line 0.

Yes, triggers a similar error in debug mode:

For example:

... o -MF lib/Demangle/CMakeFiles/LLVMDemangle.dir/ItaniumDemangle.cpp.o.d -o lib/Demangle/CMakeFiles/LLVMDemangle.dir/ItaniumDemangle.cpp.o -c /Volumes/Data/llvm-project/llvm/lib/Demangle/ItaniumDemangle.cpp
Occurence of source location from an operand does not dominate instruction.
  br label %cond.end26, !dbg !2571
!2571 = !DILocation(line: 355, column: 10, scope: !2491)
  %cond = phi i8* [ %21, %cond.true24 ], [ null, %cond.false25 ], !dbg !2571

Sorry, I missed this!

Could you please also include a testcase?

What a function :-)

I should also mention that we are actively looking into ways to upstream the swift-lldb code into llvm.org. Now that both branches are licensed under Apache the only challenge left is to make the Swift support a real language *plugin* that can be conditionally compiled.

The largest downstream LLDB fork that I know about is swift-lldb, which is also open source and thus easy to check. There also exists a Rust version, and many custom forks for various specialized hardware. Putting patches up for review here gives the maintainers a chance to voice their concerns. For larger architectural changes we usually have a thread on lldb-dev to make sure we captured everyone's attention. We're under no obligation to not break anyone's private fork, but if there is an easy/reasonable way to make everyone happy we usually tend to choose that path.

Sometimes these odd leaf APIs are used by custom downstream variants of LLDB, but without any comment pointing into that direction and no unit test, I think this makes sense to remove. Also, we can easily add it back in later.

Sounds good!

Thanks!

@shafik would you mind preparing a separate commit that add the REQUIRES everywhere it is needed, or if there are many, create an x86 subdir with an implicit requirement (like we do in the llvm/test/debuginfo tests)?
If it weren't for Rosetta, these would break on Apple Silicon machines, for example.

LGTM with that caveat

This looks nice! I'm somewhat suspicious that the new test doesn't specifically test the union case of the old test, but I'm assuming that would still work and your simpler tests covers the same code?

I'm fine with whatever numbering makes your development the easiest. My only ask is that if we still have a gap after this, please document what nodes are expected to go there, so there 1) won't be any future conflicts and 2) we don't pointlessly reserve space.

The code looks fine, I think the test needs an extra REQUIRES.

Thanks, this looks good to me — I just have a few inline comments.

@vsk We really need a richer API for MBB::erase that takes extra arguments and forces you to think about this.

I just learned that we no longer compute SDKROOT in Makefile.rules, so this seems resonable.

In D83047#2223992, @jmorse wrote:

In D83047#2208102, @vsk wrote:

Hi @jmorse, thanks for the updates on the DbgValue::{Proposed,NoVal} scheme.

I'm having trouble paging in enough of the DBG_INSTR_REF patch queue to continue giving meaningful review. I'm not sure the traditional review process is a great fit for this, since the changes are at a prototype stage. Perhaps we might be better served by landing things as they are, as a separate code path guarded behind a cl::opt? This would allow more people to test and contribute patches. Wdyt? (cc @aprantl @davide)

Ping @aprantl regarding this ^^^.

To avoid vagueness: it'd be great if this could be landed as a prototype and then refined / built up from there.

In D85988#2222616, @clayborg wrote:

Adrian: is there something I need to do to enable simulator tests? I added a test to TestSimulatorPlatform.py but if I run:

$ ../debug/bin/llvm-lit -sv lldb/test/API/macosx/simulator
llvm-lit: /Users/gclayton/Documents/src/lldb/mono/llvm-project/lldb/test/API/lit.cfg.py:147: warning: Could not set a default per-test timeout. Requires the Python psutil module but it could not be found. Try installing it via pip or via your operating system's package manager.

Testing Time: 19.61s
  Unsupported: 1

1 warning(s) in tests

It is unsupported? I am running this on a mac. Do I need to launch a simulator first? Extra arguments to the test suite?

In D83468#2224774, @dblaikie wrote:

In D83468#2224754, @aprantl wrote:

(gdb) b main
Breakpoint 1 at 0x800005ec: file test.c, line 0.

Agreed that that is not a helpful end-user experience.

How does LLDB handle this? Might also be consumer-fixable.

Thanks! Having a test would be really important to document the expected functionality and prevent future regressions.
Every time we change something here, we tend to find new unexpected behavior in a very complex LTO build a couple months later ;-)

In D86153#2225655, @Orlando wrote:

Thank you @aprantl.

I think we can (should?) remove the cut-off added in D83236 when this lands - I was testing against the same pathological case when writing this patch. Is there any kind of approval process for that?

Oh, one important thing: Can you add a section to the .rst documentation with a MIR example that explains what a DBG_VALUE_REF is / what it's used for, etc?

Mechanically this looks fine — would be good for others to chime in whether we have consensus over doing this change though.

(gdb) b main
Breakpoint 1 at 0x800005ec: file test.c, line 0.

If the problem that you want to solve is that the prologue ends at line 0, then massaging earlier passes until the right result happens to come up in one testcase is not going to be very effective. Yes, it may fix the testcase, but that won't last long, and the patch is introducing questionable behavior that will end up causing other problems.

In D82363#2216678, @scott.linder wrote:

Would it be appropriate to propose having a discussion somewhere on the topic with anyone interested, preferably on Discord or somewhere we can talk? I think we are leaning towards a very similar end-state, and I worry about just working in a silo until I can post an RFC to hash this out in text.

In D83468#2207667, @Jac1494 wrote:

In D83468#2177491, @aprantl wrote:

Thanks for posting the line table. It appears to confirm my suspicion that this patch merely hides the issue, but it isn't correct. To be sure it may be possible to preserve the debug location in this function, but implicitly setting it to the location of the instruction before it, is not the right thing to do. It may have the desired effect in your example, but it't not generally correct.

@aprantl Thank you for comment . Line llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp:2187

EntryBuilder->setDebugLoc(DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));

is setting Debug location for Line number zero , because of that AsmPrinter is getting debug location for line number zero and it is adding this entry into debug_line table .

LGTM modulo David's comment about the test.

I only added a few non-substantial inline comments.

LGTM + what @dblaikie said :-)

Looks plausible!