This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Change object::RelocationResolver to return Expected<uint64_t>
Needs ReviewPublic

Authored by MaskRay on Sep 9 2019, 2:31 AM.

Details

Summary

This will allow targets to add relocation overflow or misalignment checks.

Event Timeline

MaskRay created this revision.Sep 9 2019, 2:31 AM
ruiu added a comment.Sep 9 2019, 2:37 AM

Could you also add an actual overflow check to some target to show that this is really required? It will also make it possible to write a test for the new code.

mstorsjo added inline comments.Sep 9 2019, 2:38 AM
lld/ELF/DWARF.cpp
60

Unrelated to this (but you seem to be fixing variable lowercasing in a comment below); S here (and A in the comment below below) are also left behind upper cased here.

Relocation overflow check is probably not necessary. RelocationResolver is mainly used by DebugInfo (the use in llvm-readobj is very light) for local symbols. It is difficult to create any relocation overflow problems with meaningful tests. A somewhat clean approach to detect overflows:

+static Expected<uint64_t> reportError(const char *type, const Twine &v,
+                                      int64_t min, uint64_t max) {
+  return createStringError(inconvertibleErrorCode(),
+                           "relocation " + Twine(type) +
+                               " out of range: " + Twine(v) + " is not in [" +
+                               Twine(min) + ", " + Twine(max) + "]");
+}
+
+static Expected<uint64_t> checkInt(const char *type, int64_t v, int n) {
+  if (v == llvm::SignExtend64(v, n))
+    return v;
+  return reportError(type, Twine(v), minIntN(n), maxIntN(n));
+}

-  case ELF::R_X86_64_PC32:
-    return S + getELFAddend(R) - R.getOffset();
+  case ELF::R_X86_64_PC32: {
+    uint64_t V =  S + getELFAddend(R) - R.getOffset();
+    return checkInt("R_X86_64_PC32", V, 32);  // Passing the string literal is clumsy.
+  }

Passing the string literal to checkInt is clumsy, but the lld way may be too heavyweight?

StringRef llvm::object::getELFRelocationTypeName(uint32_t Machine,
                                                 uint32_t Type) {
  switch (Machine) {
  case ELF::EM_X86_64:
    switch (Type) {
#include "llvm/BinaryFormat/ELFRelocs/x86_64.def"
    default:
      break;
    }
    break;

I'll probably abandon this if people don't think error reporting is useful here.

I believe error handling is important. And even more important is testing of that error handling -- as I'm trying to use llvm dwarf parser in lldb I keep finding new ways to crash it with invalid input.

In D63713, we (mainly I, I guess) tried to make checking for errors less verbose by making it possible to check for DataExtractor errors in bulk via the "Cursor" class. This is the first real test of that API, and I am afraid I have to say it's not looking too good. It (I hope) made the usage of that class easier, but it requires one to be careful when writing the code "on the inside". I explain more in the inline comment.

Since you're the first person after me who has to edit this code, I'd be very interested in any feedback you have about this.

llvm/lib/DebugInfo/DWARF/DWARFDataExtractor.cpp
24

The tricky part here is that you cannot assume that the Error argument is always set, and you need to make sure to avoid tripping any Error assertions while making sure that the user still needs to check the error object. I was able to get away without doing any of this in this function, because the only way it could fail is through the getUnsigned function, which already does this tiptoe-ing. However, as you're introducing additional failure points here, you'll need to do that here as well.
The right incantation for that would be to insert:

ErrorAsOutParameter ErrAsOut(Err);
if (Err && *Err)
  return A;

here and then set the error object via if (Err) *Err = R.takeError() else consumeError(R.takeError()) below.

This means that the error will be ignored in some cases, but that's the best we can to if the user has chosen not to receive errors (which has been the only way to use the data extractor until very recently).

MaskRay updated this revision to Diff 219681.Sep 11 2019, 3:28 AM

Address review comments

MaskRay updated this revision to Diff 219683.Sep 11 2019, 3:32 AM
MaskRay marked an inline comment as done.

Address review comments

(Last update sent too fast. I still prefer multi repo but have to use a monorepo as the change touches lld - I know that the world is moving toward a monorepo - I just don't like pulling in lots of unrelevant changes with a monorepo)

MaskRay marked an inline comment as done.Sep 11 2019, 3:37 AM
MaskRay added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFDataExtractor.cpp
24

Thanks for the suggestion. (I was ready to abandon this but it looks like there is value with this change)

Do you have suggestions how I should test this change? No call site passes in the Error parameter when calling getRelocatedAddress. I am not sure what is the simplest way to let the various .debug* parser stop parsing immediately.

The test should probably use yaml2obj.