[LLDB] FindLibCppStdFunctionCallableInfo improvements #111892
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
There was a problem hiding this comment.
Thanks for putting this together! Looks like non-trivial amount of investigation.
On the first pass, I think this seems like a reasonable (and more maintainable) alternative to what we're currently doing.
Using the TypeSystem to fish out the various std::function details aligns with what we also do for the other libc++ data formatters.
Ultimately it would be nice to achieve the step-through-to-callable in a more compiler/thread-plan oriented way. But that's out of scope of this in my opinion, given we already have FindLibCppStdFunctionCallableInfo.
A high-level comment: could we split up FindLibCppStdFunctionCallableInfo? Exactly how I haven't fully thought through. But possibly a function for each of the callable cases?
I'll do a second pass of this early next week
| // pointers taking up two pointers of space. Perhaps that's why it is not legal | ||
| // to read their underlying value raw? | ||
|
|
||
| printf("Curious!"); |
There was a problem hiding this comment.
FYI, yes, it's represented as a pointer and possible offset: https://itanium-cxx-abi.github.io/cxx-abi/abi.html#member-function-pointers
There was a problem hiding this comment.
What would be the ABI agnostic way of extracting the underlying function pointer? Or is it fine to assume that the target is using the Itanium ABI?
| @@ -191,14 +191,6 @@ llvm::Error ItaniumABILanguageRuntime::TypeHasVTable(CompilerType type) { | |||
| type = pointee_type; | |||
| } | |||
|
|
|||
| // Make sure this is a class or a struct first by checking the type class | |||
| // bitfield that gets returned. | |||
| if ((type.GetTypeClass() & (eTypeClassStruct | eTypeClassClass)) == 0) { | |||
There was a problem hiding this comment.
Could you elaborate on the removal here?
There was a problem hiding this comment.
I might have overgeneralized here a bit - TypeSystemClang::IsPolymorphicClass already checks these conditions, and seems to handle situations better where CompilerType is a typedef to a struct/class instead of being one itself.
| // class __alloc_func<_Fp, _Ap, _Rp(_ArgTypes...)> { | ||
| // __compressed_pair<_Fp, _Ap> __f_; | ||
| // } | ||
| // | ||
| // class __compressed_pair : __compressed_pair_elem<_T1, 0>, | ||
| // __compressed_pair_elem<_T2, 1> { | ||
| // } | ||
| // | ||
| // struct __compressed_pair_elem { | ||
| // _Tp __value_; | ||
| // } |
There was a problem hiding this comment.
FYI, this is not the compressed pair representation of libc++ anymore. Nothing really that would affect the code here, but the comment could use an update.
There was a problem hiding this comment.
I am developing this using the SDK shipped with Xcode 16 - that's where I mostly intend to use the improvements too. Perhaps it isn't worth putting library code excerpts into comments if they are doomed to get out of sync eventually?
| if (callable_type.IsRecordType() && callable_type.GetNumFields() == 0) { | ||
| // callable_type is an empty class, and has been optimized away! Serve a dummy | ||
| callable = valobj_sp->CreateValueObjectFromAddress("__value_", | ||
| pair->GetLoadAddress(), | ||
| pair->GetExecutionContextRef(), | ||
| callable_type); |
There was a problem hiding this comment.
What do you mean optimized away here? What situation do we land here in? I assume this is not optimized code you're talking about. Why do 0 fields indicate such situation?
There was a problem hiding this comment.
I think what you were seeing actually is that libc++ removed the __compressed_pair structure. So __f_ gets you exactly the callable, not a wrapper template anymore. See e.g., what we did for the other data-formatters to support the new layout: #96538
There was a problem hiding this comment.
The code snippets above are from the SDK shipped with Xcode 16. There __compressed_pair_elem has a specialization for storing classes which are completely empty which is an empty class.
In that case empty base class elision kicks in, and leaves me with a __compressed_pair with no base classes and no fields.
Eventually I would like to make a synthetic children provider for std::function which would show the wrapped object. I figured even if it is empty it would be worthy to show a dummy of it so the UI can immediately show the underlying C++ type as well.
As far as I am aware Apple's libc++ implementation is slightly different than the one on this repo. What is the policy for such situations? Are you OK with me also supporting their layout or is that something more fitting of contributing to https://github.com/swiftlang/llvm-project?
Sure! I think a natural split could be to make a version which returns a (possibly dummy) Other code then can either proceed to extracting the load address from the One open question that I have been thinking about is how to handle callable types with multiple |
Work in progress attempt at fixing #111291. Looking for maintainer feedback/direction from @Michael137