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

[Will Hawkins 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,
----------------
hawkinsw wrote:

Sorry to follow up again, @jasonmolenda, but I think that putting an assert at the top of `PlatformDarwin::GetSharedModule` might have exposed the curiosity that I am thinking about ...

Let's say that we are on the last iteration of this loop:

```C++
  // First try all kernel binaries that have a dSYM next to them
  for (auto possible_kernel : m_kernel_binaries_with_dsyms) {
    if (FileSystem::Instance().Exists(possible_kernel)) {
      ModuleSpec kern_spec(possible_kernel);
      kern_spec.GetUUID() = module_spec.GetUUID();
      module_sp.reset(new Module(kern_spec));
```

`module_sp` is `reset` to point to a pointer to a new instantiation of a `Module`. Further, assume that

```C++
      (module_sp && module_sp->GetObjectFile() &&
          module_sp->MatchesModuleSpec(kern_spec))
```

is not true. That means that we fall out of that loop and `module_sp` still points to an instance of a `Module`. We proceed to the next loop and, for the sake of argument, let's say that `objfile_names` contains 0 entries for every `possible_kernel_dsym`. Effectively, then, there will be no iterations of that second loop. 

As a result, control will pass to `PlatformDarwin::GetSharedModule` with `module_sp` pointing to a leftover instance of a `Module` from the last iteration of the first loop. And, if you had an assertion that `module_sp` does not point to anything instead of a reset as the first operation in `PlatformDarwin::GetSharedModule` then it would certainly fail.

It seems like it should be that we reset `module_sp` before doing "the next" iteration on both of the loops in the function. I am sorry if it seems like I am being hard headed. I am just trying to understand and help! 

Thank you for your time!!


https://github.com/llvm/llvm-project/pull/78652