[Lldb-commits] [lldb] Clean up PlatformDarwinKernel::GetSharedModule, document (PR #78652)

[Jonas Devlieghere via lldb-commits]

================
@@ -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