[Lldb-commits] [PATCH] D90840: [lldb/DWARF] Fix sizes of DW_OP_const[1248][us] and DW_OP_litN results

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:944
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS));
+  auto to_generic = [addr_size = opcodes.GetAddressByteSize()](auto v) {
+    bool is_signed = std::is_signed<decltype(v)>::value;
----------------
dblaikie wrote:
> Local/non-escaping lambdas may be simpler written with default ref capture - is GetAddressByteSize() expensive or would it be fine to call it every time this lambda is called? (& capturing opcodes by ref)
GetAddressByteSize is not expensive. A default capture would work just fine.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:945
+  auto to_generic = [addr_size = opcodes.GetAddressByteSize()](auto v) {
+    bool is_signed = std::is_signed<decltype(v)>::value;
+    return Scalar(
----------------
dblaikie wrote:
> labath wrote:
> > This is probably not conforming either -- dwarf says the generic type is of "unspecified signedness", but I think it means a single global value -- not choosing signedness on a per-operand basis.
> > 
> > I've kept that for now, since that was the original behavior, though I didn't notice any issues with this bit removed.
> Fair, would be good to mark it as at least suspect/uncertain with a comment or such. (or if there's some inverse that would assert if this ever did become relevant, that assertion would be great - but I guess there probably isn't anything we can assert to check that this doesn't matter in practice)
> 
> Re: non-conforming: I think the presence of constu and consts encodings that build "generic type"d values probably means the generic type itself may have no particular notion of signedness, but the values that populate it do and I guess intend the value to be interpreted contextually or to carry the sign information? I think it's probably good to carry the sign information from the operands, at least. Maybe with some truncation/sign extension. It's probably pretty close to right... 
The DWARF standard (particularly version 4, which does not speak about signedness at all), could definitely clearer, but I think that if the committee wanted to convey the notion that a value carries the sign information with it, they would have phrased it differently. To me, the current weasel wording seems to be intended to handle the fact that different architectures have different notions about the signedness of pointers. It this world the exact DW_OP operation used does not affect the signedness of the result, but only the fact whether sign- or zero-extension takes place while converting to the "generic" type.

This is interpretation seems to match gcc behavior:
- gcc will happily use DW_OP_const1u to describe a (signed) int constant, even if that constant would fit into a DW_OP_const1s
- it will also use `DW_OP_const1s -1` as a shorter form of `DW_OP_const4u 0xffffffff` when describing `(unsigned)-1`.

In fact, these findings (and the fact that clang does not use the fixed-size operands at all) have convinced me to just fix the signedness of the generic type.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1270
     case DW_OP_const1u:
-      stack.push_back(Scalar((uint8_t)opcodes.GetU8(&offset)));
+      stack.emplace_back(to_generic(opcodes.GetU8(&offset)));
       break;
----------------
dblaikie wrote:
> Why the change to emplace_back? Generally I'd advocate for push_back where possible, since it can't invoke explicit conversions which can require more scrutiny (eg: a vector of unique_ptrs can have a unique_ptr rvalue push_back'd, but can't have a raw pointer push_back'd (a raw pointer would have to be emplace_back'd) - so when I see push_back I can assume it's a "safe" operation)
I don't know. I wanted to be fancy? Normally, I am also against the overuse of emplace_back...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90840