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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
