[PATCH] D69411: [MC] Calculate difference of symbols in two fragments when possible

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I think that this needs some wider review for a couple of reasons. It looks like this is heading towards a special case evaluation just for .if. I'm not comfortable going too much further in the approval process as this is exceeding my knowledge of MC, and I'd like see some supporting opinions on whether this is the right thing to do. Will be worth adding some more reviewers, particularly someone familiar with Mach-O. You may need an RFC to the llvm-dev mailing list to draw some attention.

I'd also like to see if there is a way of implementing this without special casing .if. That is a pragmatic solution for a specific problem, but it does make the code harder to understand so it has a cost. Looking at the reloc-diff.s test case the commit log says:

  MC/Mach-O: Use the SECTDIFF relocation type for (A - B + constant) where A is external.
   - I'm not sure why, but this is what 'as' does.

I've not been able to easily find a definition of what SECTDIFF is, but it appears to be a relocation that supports subtraction, I don't think that there is an equivalent in ELF. It maybe that the case can be resolved by not doing the folding for MachO.

some other specific comments.

- Some of the examples I gave would only fail in Thumb as Arm instructions are all the same size, whereas some Thumb-2 instructions are assembled initially as the 2-byte narrow form, and are relaxed to the wider 4-byte form if the instruction is out of range. You'll need something like -triple=thumbv7a-linux-gnueabihf to see the errors. I'd not expect some of these to fail on GNU as (that is a 2-pass assembler).
- If you can make an X86_64 test I recommend doing so as few contributors will have this optionally removed. Substantially more have ARM optionally removed, as do several buildbots.



================
Comment at: llvm/include/llvm/MC/MCExpr.h:60
+                                 const SectionAddrMap *Addrs, bool InSet,
+                                 bool IsCond) const;
 
----------------
There is a comment in evaluateAsAbsolute
```
  // Setting InSet causes us to absolutize differences across sections and that
  // is what the MachO writer uses Addrs for.
```
I think it would be useful to have something similar for IsCond.


================
Comment at: llvm/lib/MC/MCExpr.cpp:495
+  bool IsRelocatable = evaluateAsRelocatableImpl(Value, Asm, nullptr, nullptr,
+                                                 nullptr, false, true);
+
----------------
When there is more than one boolean parameter it can get difficult to track which is which, can you annotate the call sites with literal values with a comment? I've left a comment on the ones that I've noticed.

/* InSet */ true, /* IsCond */ false.


================
Comment at: llvm/lib/MC/MCExpr.cpp:521
+  bool IsRelocatable = evaluateAsRelocatableImpl(Value, Asm, Layout, nullptr,
+                                                 Addrs, InSet, false);
 
----------------
/* IsCond */ false.


================
Comment at: llvm/lib/MC/MCExpr.cpp:582
+  if (!Layout) {
+    // Try to handle cases like "foo:instr; .if . - foo == 0;instr; .endif" when
+    // the two symbols belong to two fragments in the same section.
----------------
If I've understood the code correctly I think we could be a bit more specific here:

```
// When there is no layout our ability to resolve differences between symbols is
// limited. In specific cases where the symbols are both defined in consecutive
// MCDataFragments the difference can be calculated. This is important for an
// idiom like foo:instr; .if . - foo; instr; .endif
// We cannot handle any kind of case where the difference may change due to
// layout. 
```



================
Comment at: llvm/lib/MC/MCExpr.cpp:584
+    // the two symbols belong to two fragments in the same section.
+    // FIXME: can we resolve .if conditions while finalizing layout?
+    if (IsCond && SecB.getFragmentList().getNextNode(*FragB) == FragA &&
----------------
It is difficult to see how it would be possible to resolve .if conditions at layout time in a single pass assembler. In theory the assembler could evaluate all conditional blocks and select between them at layout time, if such a layout could be converged on.


================
Comment at: llvm/lib/MC/MCExpr.cpp:702
   return evaluateAsRelocatableImpl(Res, Assembler, Layout, Fixup, nullptr,
-                                   false);
+                                   false, false);
 }
----------------
/* InSet */ false, /* IsCond */ false.


================
Comment at: llvm/lib/MC/MCExpr.cpp:708
   return evaluateAsRelocatableImpl(Res, Assembler, &Layout, nullptr, nullptr,
-                                   true);
+                                   true, false);
 }
----------------
/* InSet */ true, /* IsCond */ false.


================
Comment at: llvm/lib/MC/MCExpr.cpp:749
       if (Sym.getVariableValue()->evaluateAsRelocatableImpl(
-              Res, Asm, Layout, Fixup, Addrs, InSet || IsMachO)) {
+              Res, Asm, Layout, Fixup, Addrs, InSet || IsMachO, false)) {
         if (!IsMachO)
----------------
/* IsCond */ false.


================
Comment at: llvm/lib/MC/MCExpr.cpp:749
       if (Sym.getVariableValue()->evaluateAsRelocatableImpl(
-              Res, Asm, Layout, Fixup, Addrs, InSet || IsMachO)) {
+              Res, Asm, Layout, Fixup, Addrs, InSet || IsMachO, false)) {
         if (!IsMachO)
----------------
peter.smith wrote:
> /* IsCond */ false.
We have IsCond passed in as a parameter to evaluateAsRelocatableImpl, so even if it is passed in true, we set it to false here? Is it important for the value to be false here? If so then it implies that IsCond might not be specific enough a name. If it just doesn't matter then can we pass in IsCond here?


================
Comment at: llvm/lib/MC/MCExpr.cpp:779
     if (!AUE->getSubExpr()->evaluateAsRelocatableImpl(Value, Asm, Layout, Fixup,
-                                                      Addrs, InSet))
+                                                      Addrs, InSet, false))
       return false;
----------------
/* IsCond */ false.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69411/new/

https://reviews.llvm.org/D69411