[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

[Vedant Kumar via Phabricator via lldb-commits]

vsk added inline comments.


================
Comment at: lldb/include/lldb/Symbol/Function.h:304
+public:
+  CallEdge(const char *mangled_name, lldb::addr_t return_pc);
+
----------------
aprantl wrote:
> Does this also work for C functions? If yes, would `symbol_name` be a more accurate description?
> 
> Is this pointer globally unique in the program, or can two mangled names appear in a legal C program that don't refer to the same function?
Yes, C functions are handled. I'll use "symbol_name", hopefully that makes it clearer.


================
Comment at: lldb/include/lldb/Symbol/Function.h:304
+public:
+  CallEdge(const char *mangled_name, lldb::addr_t return_pc);
+
----------------
vsk wrote:
> aprantl wrote:
> > Does this also work for C functions? If yes, would `symbol_name` be a more accurate description?
> > 
> > Is this pointer globally unique in the program, or can two mangled names appear in a legal C program that don't refer to the same function?
> Yes, C functions are handled. I'll use "symbol_name", hopefully that makes it clearer.
Great question. No, a symbol name is not necessarily globally unique. Prior to C11, the one-definition-rule doesn't apply. Even with the ODR, a private symbol in a shared library may have the same name as a strong definition in the main executable.

I haven't tried to disambiguate ODR conflicts in this patch. What happens when a conflict occurs is: `FindFunctionSymbols` prioritizes the symbol in the main executable, and if the call edge's return PC doesn't exactly match what's in the register state we decline to create any artificial frames. I.e. in the presence of ODR conflicts, we only present artificial frames when we're 100% sure they are accurate.

Handling ODR conflicts is a 'to do', though. I'll make an explicit note of that here.


================
Comment at: lldb/packages/Python/lldbsuite/test/functionalities/tail_call_frames/ambiguous_tail_call_seq2/TestAmbiguousTailCallSeq2.py:5
+lldbinline.MakeInlineTest(__file__, globals(),
+        [decorators.skipUnlessHasCallSiteInfo])
----------------
aprantl wrote:
> This test should be restricted to only run with clang as the compiler or the -glldb flag won't work. 
> 
> I see. It is implicit in in skipUnlessHasCallSiteInfo — perhaps we should just generally add -glldb to CFLAGS? It's not a bug deal.
Yes, skipUnlessHasCallSiteInfo requires clang for now. I don't think we can add -glldb to a higher-level CFLAGS variable without breaking compiling with gcc?


https://reviews.llvm.org/D50478