[Lldb-commits] [PATCH] D121967: [LLDB][NativePDB] Create inline function decls

Wed Mar 23 12:32:17 PDT 2022

labath added a comment.

In D121967#3400827 <https://reviews.llvm.org/D121967#3400827>, @zequanwu wrote:

>> I'd consider writing the live test case in c++ (with judicious use of always_inline, noinline, etc. attributes)
>
> I think it's better to just add live test case on compiled `inline_sites.s` so the test result is not influenced by optimization change.

Well... if you write the test that way, then this is pretty much the only way to avoid random optimizer changes from braking the test.
But it has the opposite problem -- it's so strict that random changes to lldb (and its output) can break it. And since the test will only run on x86 windows, most lldb contributors (everyone except you, basically) will only notice it after it breaks.

This isn't how we usually write tests (although one could make a case doing more of it -- our coverage of debugging optimized code is fairly low). Can you say (in english) what are the properties that you are trying to check there? Maybe we can find a better way to do that...

================
Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1153
+      StringIdRecord sir;
+      cantFail(
+          TypeDeserializer::deserializeAs<StringIdRecord>(parent_cvt, sir));
----------------
rnk wrote:
> We shouldn't use cantFail if it isn't implied by local invariants. You should check if the parent scope is in fact a string first, otherwise this code will crash on invalid PDBs.
(FWIW, the amount of cantFails in the PDB code definitely makes me uncomfortable, but I don't know enough about PDBs or the PDB reader to say which of those are good invariants. But in general, we treat debug info as "user input" and try to avoid crashing on malformed data.)

================
Comment at: lldb/test/Shell/SymbolFile/NativePDB/inline_sites_live.test:2
+# clang-format off
+# REQUIRES: lld, system-windows
+
----------------
you also need `REQUIRES: native && target-x86_64`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121967