================
@@ -836,36 +834,18 @@ Status PlatformDarwinKernel::GetSharedModuleKernel(
module_sp.reset(new Module(kern_spec));
if (module_sp && module_sp->GetObjectFile() &&
module_sp->MatchesModuleSpec(kern_spec)) {
- // module_sp is an actual kernel binary we want to add.
- if (process) {
- const bool notify = false;
- process->GetTarget().GetImages().AppendIfNeeded(module_sp, notify);
- error.Clear();
- return error;
- } else {
- error = ModuleList::GetSharedModule(kern_spec, module_sp, nullptr,
- nullptr, nullptr);
- if (module_sp && module_sp->GetObjectFile() &&
- module_sp->GetObjectFile()->GetType() !=
- ObjectFile::Type::eTypeCoreFile) {
- return error;
- }
- module_sp.reset();
- }
+ if (did_create_ptr)
+ *did_create_ptr = true;
+ return {};
}
}
}
// Give the generic methods, including possibly calling into DebugSymbols
// framework on macOS systems, a chance.
- error = PlatformDarwin::GetSharedModule(module_spec, process, module_sp,
- module_search_paths_ptr, old_modules,
- did_create_ptr);
- if (error.Success() && module_sp.get()) {
- return error;
- }
-
- return error;
+ return PlatformDarwin::GetSharedModule(module_spec, process, module_sp,
----------------
JDevlieghere wrote:
Not a dumb question at all! `module_sp` is an "out parameter" and not clearing
an out parameter is a common opportunity for bugs. Whether it's a "bug" depends
on the contract of course. Per the documentation that Jason added here ("A
Module that matches the ModuleSpec, if one is found.") it should only be set
when a module is found (and assuming it applies to all the overloads) the same
expectation could be had about `PlatformDarwin::GetSharedModule`.
The obvious way to sidestep this issue is to not use out parameters at all. If
the shared pointer was the only out parameter we could've potentially
refactored this to return an `Expected<ModuleSP>` or something, but we have a
handful of other out parameters that suffer from the same problem.
Regardless, as we're using smart pointers, there's no risk of a leak. Therefore
I don't think the extra reset is warranted. If we really wanted to be
defensive, I'd prefer to use `asserts` to catch cases where we're not holding
ourselves to these kind of contracts.
https://github.com/llvm/llvm-project/pull/78652
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
