[PATCH] D76166: [MC] Handle layout change due to relaxation of another section

[Philip Reames via Phabricator via llvm-commits]

reames created this revision.
reames added reviewers: jcai19, skan, MaskRay, jyknight, annita.zhang.
Herald added subscribers: dantrushin, bollu, hiraditya, mcrosier.
Herald added a project: LLVM.

This is an alternate fix for the test case from https://reviews.llvm.org/D76114.

The (rather fundamental) issue revealed by this test case is that our relaxation implementation was not correctly adjusting the size and offsets of fragments in one section based on changes in size of another if the layout order of the two happened to be such that the former was visited before the later.  To trigger this, you need one section which refers to labels in another, but the fragments involved aren't relaxable.  (If they were, that would trigger invalidation.)  Any of fill, align, or the like can trip this.

An alternate approach would be to make all fragments with expression computable sizes relaxable.  This would require either a) storing the size in the fragment, or b) peaking ahead to compute the previously computed size from the offset of the next fragment.

I'm curious, which do folks think is less ugly?

p.s. This is mostly posted for discussion purposes.  Jain, you're welcome to take any of this code and incorporate into a refresh on the original review if desired.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76166

Files:
  llvm/lib/MC/MCAssembler.cpp
  llvm/test/MC/X86/relax-offset.s


Index: llvm/test/MC/X86/relax-offset.s
===================================================================
--- /dev/null
+++ llvm/test/MC/X86/relax-offset.s
@@ -0,0 +1,14 @@
+# RUN: llvm-mc -filetype=obj -triple=i386 %s | llvm-objdump - --headers | FileCheck %s
+
+  # CHECK: .text1        00000005 00000000 
+  # CHECK: .text2        00000005 00000000 
+
+  .section .text1
+ .skip after-before,0x0
+.Lint80_keep_stack:
+
+  .section .text2
+before:
+ jmp .Lint80_keep_stack
+after:
+
Index: llvm/lib/MC/MCAssembler.cpp
===================================================================
--- llvm/lib/MC/MCAssembler.cpp
+++ llvm/lib/MC/MCAssembler.cpp
@@ -785,10 +785,17 @@
   }
 
   // Layout until everything fits.
-  while (layoutOnce(Layout))
+  while (layoutOnce(Layout)) {
     if (getContext().hadError())
       return;
 
+    // Size of fragments in one section can depend on the size of fragments in
+    // another.  If any fragment has changed size, we have to re-layout (and
+    // as a result possibly further relax) all.
+    for (MCSection &Sec : *this)
+      Layout.invalidateFragmentsFrom(&*Sec.begin());
+  }
+    
   DEBUG_WITH_TYPE("mc-dump", {
       errs() << "assembler backend - post-relaxation\n--\n";
       dump(); });


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D76166.250318.patch
Type: text/x-patch
Size: 1250 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200313/0d7aadfc/attachment.bin>