[PATCH] D123729: [llvm-mc][macho][RFC] Enable DwarfFDESymbolsUseAbsDiff on aarch64

[Jez Ng via Phabricator via llvm-commits]

int3 created this revision.
int3 added a reviewer: m.ostapenko.
Herald added subscribers: pengfei, hiraditya, kristof.beyls.
Herald added a project: All.
int3 requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

I am trying to do this to make it easier for LLD to support EH frames
for multiple archs. At present, `DwarfFDESymbolsUseAbsDiff` (first added
[here][1]) is only enabled for x86 archs. Rather than making LLD support
both abs-diff EH frame relocations as well as regular ones, I'm hoping
we can make MC emit EH frame relocs in the same format across archs.

However, naively toggling the flag gives us an assertion error when
running MC (and deleting the assertion results in a crash). So I
reverted [the commit that added the assertion][2] and removed some
functionality. This gets me the desired output for aarch64, but causes
[this test][3] to fail instead.

>From reading commit #2's message, I suspect reverting it isn't the right
approach. Rafael authored that commit, but he's no longer around;
however the commit does reference r249303 which was also worked on by
@m.ostapenko and a mononymous "andy". I'm not sure who Andy is, so just
adding Maxim for now. Any pointers would be greatly appreciated :)

[1]: https://github.com/llvm/llvm-project/commit/618def651b59bd42c05bbd91d825af2fb2145683
[2]: https://github.com/llvm/llvm-project/commit/06c064824ef29425db785a31fed03821777fbf12
[3]: https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/AArch64/arm64_32.ll


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D123729

Files:
  llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCAsmInfo.cpp
  llvm/lib/Target/AArch64/MCTargetDesc/AArch64MachObjectWriter.cpp


Index: llvm/lib/Target/AArch64/MCTargetDesc/AArch64MachObjectWriter.cpp
===================================================================
--- llvm/lib/Target/AArch64/MCTargetDesc/AArch64MachObjectWriter.cpp
+++ llvm/lib/Target/AArch64/MCTargetDesc/AArch64MachObjectWriter.cpp
@@ -320,9 +320,33 @@
     }
 
     const MCSymbol *Base = Asm.getAtom(*Symbol);
-    // If the symbol is a variable it can either be in a section and
-    // we have a base or it is absolute and should have been expanded.
-    assert(!Symbol->isVariable() || Base);
+
+    // If the symbol is a variable and we weren't able to get a Base for it
+    // (i.e., it's not in the symbol table associated with a section) resolve
+    // the relocation based its expansion instead.
+    if (Symbol->isVariable() && !Base) {
+      // If the evaluation is an absolute value, just use that directly
+      // to keep things easy.
+      int64_t Res;
+      if (Symbol->getVariableValue()->evaluateAsAbsolute(
+              Res, Layout, Writer->getSectionAddressMap())) {
+        FixedValue = Res;
+        return;
+      }
+
+      // FIXME: Will the Target we already have ever have any data in it
+      // we need to preserve and merge with the new Target? How about
+      // the FixedValue?
+      if (!Symbol->getVariableValue()->evaluateAsRelocatable(Target, &Layout,
+                                                             &Fixup)) {
+        Asm.getContext().reportError(Fixup.getLoc(),
+                                     "unable to resolve variable '" +
+                                         Symbol->getName() + "'");
+        return;
+      }
+      return recordRelocation(Writer, Asm, Layout, Fragment, Fixup, Target,
+                              FixedValue);
+    }
 
     // Relocations inside debug sections always use local relocations when
     // possible. This seems to be done because the debugger doesn't fully
@@ -361,8 +385,19 @@
         Value -= Writer->getFragmentAddress(Fragment, Layout) +
                  Fixup.getOffset() + (1ULL << Log2Size);
     } else {
-      llvm_unreachable(
-          "This constant variable should have been expanded during evaluation");
+      // Resolve constant variables.
+      if (Symbol->isVariable()) {
+        int64_t Res;
+        if (Symbol->getVariableValue()->evaluateAsAbsolute(
+                Res, Layout, Writer->getSectionAddressMap())) {
+          FixedValue = Res;
+          return;
+        }
+      }
+      Asm.getContext().reportError(Fixup.getLoc(),
+                                  "unsupported relocation of variable '" +
+                                      Symbol->getName() + "'");
+      return;
     }
   }
 
Index: llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCAsmInfo.cpp
===================================================================
--- llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCAsmInfo.cpp
+++ llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCAsmInfo.cpp
@@ -46,6 +46,7 @@
   UsesELFSectionDirectiveForBSS = true;
   SupportsDebugInformation = true;
   UseDataRegionDirectives = true;
+  DwarfFDESymbolsUseAbsDiff = true;
 
   ExceptionsType = ExceptionHandling::DwarfCFI;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D123729.422630.patch
Type: text/x-patch
Size: 3174 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220413/00cb7fa6/attachment.bin>