dblaikie added a comment. In D94063#2481867 <https://reviews.llvm.org/D94063#2481867>, @labath wrote:
> The fix is good, but the test could be improved. Yeah - haven't written lldb patches before so totally open to suggestions on the testing front for sure. Thanks! > Combining assembly input with running the inferior effectively limits the > test to a single platform (assembly is not portable, and running requires asm > to match the host). If it's better to write it using C++ source and custom clang flags I can do that instead (it'll be an -mllvm flag - looks like there's one other test that does that: `lldb/test/API/lang/objc/forward-decl/TestForwardDecl.py: dict(CFLAGS_EXTRAS="-dwarf-version=5 -mllvm -accel-tables=Dwarf"))`) - means the test will be a bit more convoluted to tickle the subprogram ranges, but not much - just takes two functions+function-sections. > AFAICT, we don't actually need to run the binary to test this fix -- checking > just the "breakpoint set" command should suffice (if you want to be more > explicit in what is being checked, you can also run "breakpoint list -v" and > test that). Then you'd just need to replace `%clang_host` with `%clang > -target x86_64-pc-linux` (or something) and `UNSUPPORTED: system-windows` > with `REQUIRES: x86`. Yeah, I was hoping to setup the test for testing both these things in a uniform way, but if best practice is to test them with different mechanisms I'm OK with that. > I'll write about variable printing in the other review. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94063/new/ https://reviews.llvm.org/D94063 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
