[Lldb-commits] [PATCH] D60172: Renamed Target::GetSharedModule to AddModule, allow for ModulesDidLoad to be delayed when batch adding Modules

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

OK looked things over a bit.

I think there's a case for adopting llvm::ErrorOr here, and elsewhere across, but today the only place we're using that return type is when the error is encoded in a std::error_code (aka fancy errno).  We should have an ErrorOr that uses a Status object to contain the error, I think that's possible?  I've seen people use their own objects to return errors with llvm::Expected.  In any event, I think a change like this is a little out of scope for what I'm doing in this patch right now.

After spending time looking at all the error handling codepaths related to AddOrCreateModule, if we simply fail to find a binary we return an empty ModuleSP and the platform may set a Status with an error string (e.g. PlatformPOSIX will say no such file) if that's non-nullptr.  Or there may be a genuine error - a Target that doesn't have a Platform, we found a binary but it is an explicitly disallowed type (e.g. eTypeStubLibrary, a long-obsolete binary format on Darwin), we can return an error message explaining that to the caller if the Status ptr is non-nullptr.

The important wrinkle with AddOrCreateModule is that most of the callers are deep inside lldb internal mechanisms -- the DynamicLoaders.  And for most of those, we're going to handle file-not-found in custom ways, we're not going to relay the error messages.  For instance, the user-process DynamicLoaderDarwin, when it fails to find a file, it will call Process::ReadModuleFromMemory().  If the kernel debugging DynamicLoaderDarwinKernel fails to load a kext (a solib), it accumulates a list of kexts that it could not find, and at the end it will present a sorted list of them.   These callers have no interest in the returned error message.

On the other hand, a caller like CommandObjectTarget, which is directly driven by user interaction, we must print the error message ("file not found" and such).

So in this particularly API, because only some callers want the error strings, leaving this as an optional argument seems reasonable.  An llvm::ErrorOr which returns a ModuleSP or a Status would be a fine - and only the callers that want to print the error message would retrieve the Status object, the others would do their normal behavior when couldn't get the ModuleSP.


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60172/new/

https://reviews.llvm.org/D60172