[Lldb-commits] [PATCH] D121631: Introduce new symbol on-demand for debug info

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/test/Shell/SymbolFile/OnDemand/shared-lib-globals-hydration.test:4
+
+# REQUIRES: system-linux
+
----------------
yinghuitan wrote:
> labath wrote:
> > yinghuitan wrote:
> > > labath wrote:
> > > > Is this here because there is no portable way to create shared libraries in shell tests? Would it be possible to run this test on other systems as well if this were written as an API test (where we have a shared lib portability layer)?
> > > Yeah, that's one reason. Another reason is the name of the shared library (used for checking global variables/debug info) varies across platform. For example, it can be libfoo.dylib (Mac) vs libfoo.so (linux) vs *foo*.dll (windows). I find it cumbersome to support multiple platforms. 
> > I'm not saying you have to go out of your way to support other platforms and their idiosyncrasies, but the differences in library names hardly seem like an insurmountable obstacle. We already have abstractions in the API tests (`self.platformContext.{shlib_prefix,shlib_extension}`) which exist precisely for this reason. There's no function to automatically construct the library name (you have to do the concatenation yourself), but I certainly wouldn't be opposed to adding one.
> > 
> > If it helps, think of it as courtesy towards other developers who may need to debug this feature when they break these tests -- that's much easier to achieve if you can reproduce the failure locally vs. just getting a failure email from a random bot (after you've already submitted your patch).
> > self.platformContext.{shlib_prefix,shlib_extension}
> 
> Ah, did not know their existence, thanks! Will add API tests for them. Do you prefer to keep these linux-only shell tests or completely deprecate them?
> 
> 
If they're going to be testing the exact same thing, then I don't see a reason for keeping these around.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121631