[Lldb-commits] [PATCH] D138060: Improve error logging when xcrun fails to execute successfully

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.


================
Comment at: lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm:13
 #include "lldb/Host/macosx/HostInfoMacOSX.h"
+#include "lldb/Core/Debugger.h"
 #include "lldb/Utility/Args.h"
----------------
aprantl wrote:
> labath wrote:
> > We shouldn't have Host->Core dependencies. Could you report the error in the caller?
> How strong to you fell about this?
> It would be an odd interface. `GetXcodeSDKPath` also caches negative results because negative lookups are really slow. This means we would only raise an error on the first call and not on the subsequent ones. I can do that, but it also feels weird. I guess we could cache the error string as well?
I would say pretty strongly. While we still have cycles in LLDB, we haven't introduced new one,  at least not that I'm aware off. The diagnostics can be fixed by percolating the error up further and dealing with the errors at the call site, which presumably can depend on Host. Caching the error sounds reasonable to me. 

That approach doesn't work for the progress updates though. I can imagine that there's more places in LLDB that don't rely on Core from where we'd like to report progress. One potential solution to that is moving Progress to Utility and replacing the dependency on Core with a callback. Similar to the diagnostics we could make it something that follows the Initialize/Terminate patter and then keeps a static list of callbacks for reporting progress. Definitely not the most elegant solution, but it could work for both Progress and Diagnostics if we wanted to. 


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

https://reviews.llvm.org/D138060