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

[David Blaikie via Phabricator via lldb-commits]

dblaikie 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;
----------------
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)


================
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(
----------------
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... 


================
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;
----------------
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)


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1293-1300
+    // These should also use to_generic, but we can't do that due to a
+    // producer-side bug in llvm. See llvm.org/pr48087.
     case DW_OP_constu:
-      stack.push_back(Scalar(opcodes.GetULEB128(&offset)));
+      stack.emplace_back(opcodes.GetULEB128(&offset));
       break;
     case DW_OP_consts:
+      stack.emplace_back(opcodes.GetSLEB128(&offset));
----------------
Seems fair


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90840