labath added a comment.

I agree with everything Jim said.

One of the interesting 'strategy' questions would be the interaction between 
this feature and the `target.preload-symbols` feature. I don't think the 
interaction between the two is obvious or intuitive, and I'm even sure that 
supporting all possible combinations is useful. We could e.g. consider 
replacing the two settings with one ternary setting with values like "load 
everything upfront", "load everything the first time it's needed" and "load 
everything when the module is accessed in a specific way".

Also, I don't think that "run everything with the setting turned on" is a good 
testing strategy. Nearly all of our tests consist of a single binary and the 
very first action they do is set a file/line or a function breakpoint. That 
will immediately trigger a 'hydration' and the rest of the test will just check 
that the forwarders really forward all calls. Rerunning all tests to catch the 
few testcases that do something interesting is not a good tradeoff.

We should identify the interesting/important scenarios and write targeted tests 
for that. The tests in this patch are a pretty good start. My only complaint is 
that the shared library tests are linux-only. We currently don't have 
infrastructure in place to write shell tests with shared libraries portably. It 
may be worth rewriting those into api tests to make use of the existing shared 
library facilities.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121631

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to