This is an archive of the discontinued LLVM Phabricator instance.

Fixing DWARFExpression handling of ValueType::FileAddress case for DW_OP_deref_size
ClosedPublic

Authored by shafik on Mar 10 2022, 1:14 PM.

Details

Summary

Currently DW_OP_deref_size just drops the ValueType::FileAddress case and does not attempt to handle it. This adds support for this case and a test that verifies this support.

I did a little refactoring since DW_OP_deref and DW_OP_deref_size have some overlap in code.

Diff Detail

Event Timeline

shafik created this revision.Mar 10 2022, 1:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 1:14 PM
shafik requested review of this revision.Mar 10 2022, 1:14 PM

There can probably be some more refactoring wrt to the Value class which also does a lot of the same work but there are enough differences that I think any attempt at that should be left for a separate PR.

This is nice, I have a few mostly stylistic comments inline.

lldb/source/Expression/DWARFExpression.cpp
946

Maybe add a Doxygen comment explaining what this function does?

947

Should this also be in the anonymous namespace?

963

return so_addr.GetLoadAddress(exe_ctx->GetTargetPtr();
or load_addr

976

the break is redundant after the return.

989

This shouldn't be necessary, or does clang warn about the function not having a return on this otherwise?

1167–1168

why is Addr captialized?

1167–1168

we sometimes call these maybe_load_addr or load_addr_or_err not sure if that's better :-)

1167–1168

should we sink this check into ResolveAndLoadFileAddress, too?

1298

Comment what this case handles? It's unintuitive that we go into a case where load_Addr == LLDB_INVALID_ADDRESS

1302

Why do we need to force live memory?

1313

It would be nice to invert the if and have an early exit instead.

1319

same here, but maybe we can just sink this into the helper.

aprantl added inline comments.Mar 10 2022, 2:33 PM
lldb/test/Shell/SymbolFile/DWARF/x86/DW_OP_deref_size_static_var.s
2

Out of curiosity: Does the x86 directory's lit.cfg require a compiler with with an x86 *backend*, or an x86 *host*?

shafik updated this revision to Diff 414663.Mar 11 2022, 8:01 AM
shafik marked 12 inline comments as done.

Addressing comment

  • Adding documentation
  • Moving some more error handling into helper
  • Cleaning up the calling side
shafik added inline comments.
lldb/source/Expression/DWARFExpression.cpp
947

Good catch although static is probably more appropriate: https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

1167–1168

load_Addr was what it was before, I didn't even notice it.

1302

Good catch, we don't.

1319

Makes sense, it was a little tricky but I think with these changes it is neater.

aprantl accepted this revision.Mar 11 2022, 9:07 AM

Thanks!

lldb/source/Expression/DWARFExpression.cpp
961

Thanks! I would have been happy with just the function description ;-)

1361–1362

Why is this commented out?

This revision is now accepted and ready to land.Mar 11 2022, 9:07 AM
shafik updated this revision to Diff 415440.Mar 15 2022, 8:06 AM
shafik marked an inline comment as done.

Removing dead code.

shafik updated this revision to Diff 415473.Mar 15 2022, 9:31 AM

Fixing up diff

Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2022, 9:36 AM

This change uses a variable-sized array, which is c99 only:

third_party/llvm/llvm-project/lldb/source/Expression/DWARFExpression.cpp:1300:30: error: variable length arrays are a C99 feature [-Werror,-Wvla-extension]
          uint8_t addr_bytes[size];
                             ^~~~
third_party/llvm/llvm-project/lldb/source/Expression/DWARFExpression.cpp:1300:30: note: read of non-const variable 'size' is not allowed in a constant expression
third_party/llvm/llvm-project/lldb/source/Expression/DWARFExpression.cpp:1242:15: note: declared here
      uint8_t size = opcodes.GetU8(&offset);

But I the size of the array is limited to only to the address size of the target machine, so converting that to a simple eight-byte array and then using size instead of sizeof(addr_bytes) should be fine. I have a change in testing to do that.

Fix for the variable-sized-array issue in rG7518e0ff63cd

Sorry for the late comments. Looks like it got checked in already, but will submit inline comments anyway.

lldb/source/Expression/DWARFExpression.cpp
963

Pass the module_sp in as a reference to avoid the increment/decrement on the reference count?

963

Maybe "ResolveLoadAddress" would be a better name?

971

Do we prefer "return {}" over "return llvm::None"?

983

Not sure if you need to do this as the call to "module_sp->ResolveFileAddress(...) will return false if the address wasn't resolved to a address that is section offset.

1006–1017

DataExtractor already has a GetMaxU64() call you can use. It handles anything <= 8 bytes in size. Do we really want to handle the "default:" case above or the "else" clause in my suggested fix though?

1102

revert

aprantl added inline comments.Mar 18 2022, 4:10 PM
lldb/source/Expression/DWARFExpression.cpp
971

Personally, I do, but there's no rule about that.