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
947

Maybe add a Doxygen comment explaining what this function does?

948

Should this also be in the anonymous namespace?

964

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

977

the break is redundant after the return.

990

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

1168–1169

why is Addr captialized?

1168–1169

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

1168–1169

should we sink this check into ResolveAndLoadFileAddress, too?

1299

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

1303

Why do we need to force live memory?

1314

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

1320

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
948

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

1168–1169

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

1303

Good catch, we don't.

1320

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
962

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

1362–1363

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
964

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

964

Maybe "ResolveLoadAddress" would be a better name?

972

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

984

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.

1007–1018

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?

1103

revert

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

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