[Lldb-commits] [PATCH] D126359: [LLDB] Add basic floating point ops to IR interpreter

[Pavel Kosov via Phabricator via lldb-commits]

kpdev42 added inline comments.


================
Comment at: lldb/test/API/lang/c/fpeval/TestFPEval.py:32
+        self.runCmd("run", RUN_SUCCEEDED)
+        self.expect("expr --allow-jit false  -- a + b", VARIABLES_DISPLAYED_CORRECTLY,
+                    substrs=['double', '44'])
----------------
clayborg wrote:
> You might be able to get away with not actually even creating a target or running to main if you define variables in your expression:
> 
> ```
> self.expect('expr --allow-jit false  -- float x=2.2; float y=4.4; x+y', ...
> ```
Actually I wanted the test to read floating point values from process memory, checking if they're passed correctly to IR along the way. This might be redundant, however your expression still needs a target to be executed.


================
Comment at: lldb/test/API/lang/c/fpeval/TestFPEval.py:32-33
+        self.runCmd("run", RUN_SUCCEEDED)
+        self.expect("expr --allow-jit false  -- a + b", VARIABLES_DISPLAYED_CORRECTLY,
+                    substrs=['double', '44'])
+        self.expect("expr --allow-jit false  -- a - b", VARIABLES_DISPLAYED_CORRECTLY,
----------------
clayborg wrote:
> if we want to verify if the above "a+b" works as expected compared to JITed code, you can also run an expression like:
> 
> ```
> expr eval(a, b, add)
> ```
> Then then we would want to compare the expression results to make sure the binary answer matches exactly. To do this, we will want to use the LLDB native APIs:
> ```
> no_jit_options = lldb.SBExpressionOptions()
> no_jit_options = expt_options.SetAllowJIT(False)
> jit_options = lldb.SBExpressionOptions()
> jit_options = expt_options.SetAllowJIT(True)
> no_jit_value = frame.EvaluateExpression("a+b", no_jit_options)
> jit_value = frame.EvaluateExpression("eval(a, b, add)", jit_options)
> no_jit_data = no_jit_value.GetData()
> jit_data = no_jit_value.GetData()
> ```
> Then we need to compare the data byte for byte.
> 
> 
> 
IMO this will not work, because lldb always tries interpreter first and jitter is used only if interpreter fails. This may be circumvented by generating expression which interpreter cannot handle (function call?), however I don't see a point in doing it. Why not just check result value?


================
Comment at: lldb/test/API/lang/c/fpeval/main.c:3-4
+{
+    double a = 42.0;
+    double b = 2.0;
+    return 0; //// Set break point at this line.
----------------
clayborg wrote:
> We should be testing "float" and "long double" as well to verify those work.
Long double is not supported by this patch, but can be added. However long double is platform dependent type, so it makes no sense at all comparing JIT and interpreted results


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126359