This patch has no effect for C and C++. In more dynamic languages, such as Objective-C and Swift GetByteSize() needs to call into the language runtime, so it's important to pass one in where possible. My primary motivation for this is some work I'm doing on the Swift branch, however, it looks like we are also seeing warnings in Objective-C that this may resolve.
Everything in the SymbolFile hierarchy still passes in nullptrs, because we don't have an execution context in SymbolFile, since SymbolFile transcends processes.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
lldb/source/Plugins/ABI/ARC/ABISysV_arc.cpp | ||
---|---|---|
461 | Above you use &thread in return_compiler_type.GetByteSize(&thread) ... why do both of these work? |
lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp | ||
---|---|---|
308 | Is this equivalent to what was being done previously or is this better in some way? |
lldb/source/Plugins/ABI/ARC/ABISysV_arc.cpp | ||
---|---|---|
461 | This doesn't compile (but ARC isn't a default target, so probably no one compiled that code). |
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp | ||
---|---|---|
4092 | So we already have this dubious stride extra-output here, but now it also requires us to have an execution context parameter. I think we might as well just remove that parameter (in a separate patch). |
lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp | ||
---|---|---|
308 | The old code took a TargetSP, wrapped it in an ExecutionContext, and then called GetBestExecutionContextScope(), which returns the Target *. (Target inherits from ExecutionContextScope) So the new code does exactly the same thing as the old code but it's much shorter. | |
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp | ||
4092 | Are you saying no one uses stride? Not even a future Fortran lldb Plugin? |
I pointed out a couple of places where you have either at hand or near at hand a useful exe_ctx that you could pass instead of nullptr.
Other than that, this seems good to me. Thanks for pushing this through.
lldb/source/API/SBType.cpp | ||
---|---|---|
210 | This changes makes it obvious we should add SBType::GetByteSize(SBExecutionContext &exe_ctx) but I don't think you need to do that in this patch. | |
lldb/source/Commands/CommandObjectTarget.cpp | ||
1656 | LookupTypeInModule is called by LookupInModule which is only ever called when we have a target on hand. Maybe it would be better to thread the target through so we can get better type descriptions if the target has a live process? Or make it a member function of CommandObjectTargetModulesLookup and it could use m_exe_ctx. It doesn't seem to be gaining anything by being a stand-alone function. | |
1701–1702 | Ditto with this one. Our only caller (the method called LookupTypeHere) definitely had a better ExecutionContextScope (from m_exe_ctx). Be nice to pass it in here. | |
lldb/source/DataFormatters/FormatManager.cpp | ||
240–242 | We have the ValueObject we are working on in this scope. ValueObjects always have an ExecutionContextRef (GetExecutionContextRef) so you can pass in a better ExecutionContextScope here. |
This changes makes it obvious we should add
SBType::GetByteSize(SBExecutionContext &exe_ctx)
SBType::GetArrayElementType(SBExecutionContext &exe_ctx)
but I don't think you need to do that in this patch.