[Lldb-commits] [PATCH] D131974: [lldb][ClangExpression] Add asm() label to all FunctionDecls we create from DWARF

Fri Aug 19 05:40:27 PDT 2022

labath added a comment.

In D131974#3733024 <https://reviews.llvm.org/D131974#3733024>, @Michael137 wrote:

> In D131974#3732727 <https://reviews.llvm.org/D131974#3732727>, @labath wrote:
>
>> Any particular reason for not removing the `SC_Extern` check? I'm pretty sure I could create a test case with a `static` abi-tagged function which we wouldn't be able to call with that check in place...
>
> Boiled down to this comment:
>
>> the local (indicated by the `L` in the name) mangled name is not necessarily unique
>
> Attaching it to non-external functions caused `cpp/namespace/TestNamespaceLookup.py` to fail. With a non-static and a static version of `func()` LLDB chose the wrong one. I can follow up on whether this is a bug elsewhere and how feasible it would be to unconditionally attach the label

Ah, this is a fun one. So this breaks the `test_scope_lookup_with_run_command` test in `TestNamespaceLookup.py`, but simultaneously "fixes"  `test_file_scope_lookup_with_run_command`. I would say this is a pre-existing bug, that just happens to manifest itself differently with this change. The problem is that lldb is not able to correctly scope CU-local objects, as it ends up putting all files into one huge AST context. It was mostly accidental that lldb picked the external version of the function (because it happened to get its name mangling "right"). Now it happens to pick the static one as they both have the same signature (and I guess this is the one that gets parsed first).

I'd say we should just flip the xfails around, since this is already pretty broken, and the asm labels definitely help in some situations.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131974/new/

https://reviews.llvm.org/D131974