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

Fri Aug 19 06:30:55 PDT 2022

Michael137 added a comment.

In D131974#3734849 <https://reviews.llvm.org/D131974#3734849>, @labath wrote:

> 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).

When I initially looked at, it seemed unintuitive for LLDB to pick the file-static function (from a context where it's usually not accessible) over the one in the header so I kept the check in.
But it's true that the storage-class just works around a different bug. There seem to always be cases where we pick an incorrect overload no matter how you flip it.
Happy to remove the check and flip the XFAIL

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131974