[MC] Recursively calculate symbol offset

[lgrey]

Summary

This is speculative since I'm not sure if there's some implicit contract that a variable symbol must not have another variable symbol in its evaluation tree.

Downstream bug: https://bugs.chromium.org/p/chromium/issues/detail?id=471146#c23.

Test is based on alias.s (removed checks since we just need to know it didn't crash).

[MaskRay]

With a recursive getSymbolOffsetImpl, I am worried about infinite loop if two symbols are variables referencing each other (it seems impossible, but symbol reassignments can sometimes happen, unfortunately the conditions are pretty complex, so the risk exists).

chained-alias-offset.s works for ELF where l_c is l_a + 1.

So we should understand why it fails on Mach-O.

[lgrey]

I think the root cause is: https://bugs.llvm.org/show_bug.cgi?id=19203. The proximate cause is https://github.com/llvm/llvm-project/blob/main/llvm/lib/MC/MCExpr.cpp#L820.

In the real case that motivated this, we have something like BinaryExpr(SymRefExpr(BinaryExpr(SymRefExpr(Constant) + Constant) + Constant)). Without the MachO special case in MCExpr, this would get reduced to an expression without nested symbols before it makes it to the offset calculation.

[MaskRay]

Ah, right. I looked at this piece of code and wondered as well last year...

I looked at some ninja check-llvm-mc failures if we remove the hack:

--- i/llvm/lib/MC/MCExpr.cpp
+++ w/llvm/lib/MC/MCExpr.cpp
@@ -812,6 +812,7 @@ bool MCExpr::evaluateAsRelocatableImpl(MCValue &Res, const MCAssembler *Asm,
                                                    Kind, Asm->getContext()),
                            Res.getSymB(), Res.getConstant(), Res.getRefKind());
         }
+        return true;
         if (!IsMachO)  /// a proper fix should remove this block completely
           return true;

For

• diff-with-two-sections.s
• variable-exprs.s

I guess removing the hack may fix bugs?!

For eh-frame-reloc.s, there will be a pair of X86_64_RELOC_SUBTRACTOR __eh_frame and X86_64_RELOC_UNSIGNED _bar which could(?) be cancelled but not by MC. I think this is benign.

My Mach-O knowledge is fairly insufficient.

@lhames Can you check if the hack can be deleted? If it works, please just push the change:) Then this patch can be abandoned.

[lgrey]

So I tried this and the eh_frame thing is a real problem at link time. lld errors because the X86_64_RELOC_SUBTRACTOR ends up being local. The link succeeds in ld64, but produces a weirdly large binary.

[lgrey]

lhames@ ping :) Any idea what the right approach is here?

[lhames]

I think the MachO behavior described in https://bugs.llvm.org/show_bug.cgi?id=19203 is broken, but I still don't think that we can change this until we have a plan to deal with instances of the buggy pattern in the wild -- that's a much bigger issue than underlying bug.

[lgrey]

In D109109#3013769, @lhames wrote:

I think the MachO behavior described in https://bugs.llvm.org/show_bug.cgi?id=19203 is broken, but I still don't think that we can change this until we have a plan to deal with instances of the buggy pattern in the wild -- that's a much bigger issue than underlying bug.

What's a good path forward to fix that?

[lhames]

In D109109#3015293, @lgrey wrote:

In D109109#3013769, @lhames wrote:

I think the MachO behavior described in https://bugs.llvm.org/show_bug.cgi?id=19203 is broken, but I still don't think that we can change this until we have a plan to deal with instances of the buggy pattern in the wild -- that's a much bigger issue than underlying bug.

What's a good path forward to fix that?

I don't thing there's a good path. Given that in-tree assembly is often associated with sensitive code (e.g. kernels and drivers) I think that we would have to put significant effort into making any changes in behavior visible. I can think of two long, awkward paths for this:

(1) We introduce an assembly directive that describes the expected assignment behavior: .llvm_pr19203=fixed|compat. On MachO, we could emit a warning when we encounter a buggy expression if that directive is unset, or set to compat: "Warning: Using llvm_pr19203 compat mode. Please audit or or regenerate your assembly, then switch to fixed". This directive would probably hang around forever (once it's added to the source it's going to be hard to get rid of again).

(2) We rely on assembler tool options, rather than a directive. We lose the ability to determine intended behavior from the source, but don't have to keep an oddball directive around forever. The option would go through a few phases:
Initially: --pr19203=compat (by default) -- If omitted or explicitly set to compat then this will issue the same warning as above. Explicitly setting '--pr19203=fixed' will silence the warning.
Then: --pr19203=fixed (by default) -- If set to compat this will issue a warning that pr203 compat support is going away, and the code should be urgently reviewed/fixed. If set to fixed this should issue a warning: "--pr19203=fixed is now the default. Please remove this option from your assembler invocation".
Sunset: --pr19203 generates an error if set to compat ("compat support is gone"). In 'fixed' mode it now warns that the option should be removed urgently, as the option itself is going away.
Finally: --pr19203 option is removed.

Given how infrequently some groups update compilers the length between the phases should probably be measured in years. This is especially true between 'Initially' and 'Then', when we change the default behavior.

There may be problems with these approaches that I haven't considered too -- either one would need to be discussed with the wider community before work started.

Are there any workarounds that you could apply to solve your issue? If so I'd be inclined to try those rather than tackling this.

[lgrey]

Oof! That does sound prohibitively difficult.

@MaskRay is the mutual reference issue that you referred to tractable?

[MaskRay]

In D109109#3025650, @lgrey wrote:

Oof! That does sound prohibitively difficult.

@MaskRay is the mutual reference issue that you referred to tractable?

@lgrey I am just concerned that some configurations of the Linux kernel's Clang build may hit a behavior change here (I do remember symbol reassignments from some assembly files), but it is possible that I am overly worried.

@nickdesaulniers may assist on testing the patch (with just the llvm/lib/MC/MCFragment.cpp change)

In any case, the llvm/lib/MC/MCFragment.cpp changes need a FIXME comment that it works around the Mach-O issue and is not needed for other binary formats.

---

(1) We introduce an assembly directive that describes the expected assignment behavior: .llvm_pr19203=fixed|compat. On MachO, we could emit a warning when we encounter a buggy expression if that directive is unset, or set to compat: "Warning: Using llvm_pr19203 compat mode. Please audit or or regenerate your assembly, then switch to fixed". This directive would probably hang around forever (once it's added to the source it's going to be hard to get rid of again).

I think such a Mach-O specific diagnostic will be useful in evaluateAsRelocatableImpl. Hope either of you (who work on Mach-O :) can implement it.

(2) We rely on assembler tool options, rather than a directive. We lose the ability to determine intended behavior from the source, but don't have to keep an oddball directive around forever. The option would go through a few phases:

LGTM, too. This can avoid FIXME in this patch.
If my assessment that "I guess removing the hack may fix bugs?!" is correct, I actually prefer this approach.

[Gerolf]

Agree on the painpoints, but from the user perspective Lang's proposal (2) is a considerate approach to handle this.

[nickdesaulniers]

I built the mainline Linux kernel for x86_64, arm, and arm64 (defconfigs) with clang's integrated assembler with this patch without any problem. I also boot tested the resulting images in qemu. So LGTM from that perspective, but please wait for additional sign off from other reviewers.

[lgrey]

@MaskRay are you OK with this (with added FIXME) given the comment above? I think just removing the hack (and doing the migration work) isn't enough due to the issues I saw with relocations after removing it.

[MaskRay]

Please add a comment and run arc diff 'HEAD^' or manually upload the diff.
The diff as it currently displays cannot be accepted.

llvm/test/MC/MachO/chained-alias-offset.s

2

Use llvm-readobj -s to check the symbol values, instead of checking it doesn't crash.

[MaskRay]

llvm/test/MC/MachO/chained-alias-offset.s

3

Test discovery trick: update MCFragment.cpp to use a wrong value and run check-llvm-mc

Consider adding the new test to an existing file.

[lgrey]

Added comment, updated test

[lgrey]

llvm/test/MC/MachO/chained-alias-offset.s

3

Unfortunately, that breaks a great deal of tests. I glanced through the broken Mach tests specifically and didn't find anything that looks like a good fit.

llvm/lib/MC/MCFragment.cpp

if (!S.getVariableValue()->evaluateAsValue(Target, Layout))
     report_fatal_error("unable to evaluate offset for variable '" +
                        S.getName() + "'");
 
   uint64_t Offset = Target.getConstant();
 
   const MCSymbolRefExpr *A = Target.getSymA();
   if (A) {
     uint64_t ValA;
-    if (!getLabelOffset(Layout, A->getSymbol(), ReportError, ValA))
+    // FIXME: On most platforms, `Target`'s component symbols are labels from
+    // having been simplified during evaluation, but on Mach-O they can be
+    // variables due to PR19203. This, and the line below for `B` can be
+    // restored to call `getLabelOffset` when PR19203 is fixed.
+    if (!getSymbolOffsetImpl(Layout, A->getSymbol(), ReportError, ValA))
       return false;
     Offset += ValA;
   }
 
   const MCSymbolRefExpr *B = Target.getSymB();
   if (B) {
     uint64_t ValB;
-    if (!getLabelOffset(Layout, B->getSymbol(), ReportError, ValB))
+    if (!getSymbolOffsetImpl(Layout, B->getSymbol(), ReportError, ValB))
       return false;
     Offset -= ValB;
   }
 
   Val = Offset;
   return true;
 }
 

llvm/test/MC/MachO/chained-alias-offset.s

+// RUN: llvm-mc -triple x86_64-apple-macos %s -filetype=obj | llvm-readobj --symbols - | FileCheck %s
+l_a:
+l_b = l_a + 1
+l_c = l_b
+        .long l_c
+
+// CHECK: Name: l_a
+// CHECK: Value: 0x0
+// CHECK: Name: l_b
+// CHECK: Value: 0x1
+// CHECK: Name: l_c
+// CHECK: Value: 0x1