[Lldb-commits] [PATCH] D67227: [lldb] Extend and document TestIRInterpreter.py

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.


================
Comment at: lldb/packages/Python/lldbsuite/test/commands/expression/ir-interpreter/TestIRInterpreter.py:54
+                # Shifting longer than size of a type also doesn't work.
+                if rhs.value <= 0 or rhs.value >= 7:
+                    return False
----------------
shafik wrote:
> teemperor wrote:
> > shafik wrote:
> > > I may be missing something here but this looks like it should be `32` instead of `7` or rather `sizeof(int)*8`
> > > 
> > > When we say doesn't work do we mean undefined behavior?
> > Good catch, originally that was supposed to be `> 7` so that we don't overflow any data type (assuming we ever extend the test to char). But I can change it to `>= 32` until we actually use any 8-bit type.
> > 
> > And 'doesn't work' means that it will literally cause the test to fail and this test stops working. The interpreter will do something else than the JIT in these cases which is a bug. We probably should detect UB when interpreting these expressions and throw an error, but that's a whole new story. This is more about adding testing to the existing code.
> if I am not missing anything here in C++ and C the operands of expressions undergo the usual arithmetic conversions and for integral types they undergo integer promotions. This means that the smallest type should be `int` or `unsigned int`.
> 
> [More details](https://stackoverflow.com/a/24372323/1708801).
Done, thanks!


================
Comment at: lldb/packages/Python/lldbsuite/test/commands/expression/ir-interpreter/TestIRInterpreter.py:110
+                variable_list.append(v)
+                interp_result = self.frame().EvaluateExpression(
+                    v.decl_expr, nojit_options).GetValueAsUnsigned()
----------------
shafik wrote:
> You don't see to use `interp_result` here
Yeah, seems that declaring a $variable in LLDB just returns some nonsensical value and a meaningless error... We will know if we have an error later, but at the moment there doesn't seem to be any early error checking we can do here.


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

https://reviews.llvm.org/D67227