Skip to content

Conversation

MQ-mengqing
Copy link
Contributor

The shouldInsertExtraNopBytesForCodeAlign() need STI to check whether relax is enabled or not. It is initialized when call setEmitNops. The setEmitNops may not be called in a section which has instructions but is not executable. In this case uninitialized STI will cause problems. Thus, check hasEmitNops before call it.

Fixes: #76552 (comment)

…rCodeAlign

The shouldInsertExtraNopBytesForCodeAlign() need STI to check whether
relax is enabled or not. It is initialized when call setEmitNops. The
setEmitNops may not be called in a section which has instructions but
is not executable. In this case uninitialized STI will cause problems.
Thus, check hasEmitNops before call it.

Fixes: llvm#76552 (comment)
@llvmbot llvmbot added the llvm:mc Machine (object) code label Jan 7, 2024
@llvmbot
Copy link
Member

llvmbot commented Jan 7, 2024

@llvm/pr-subscribers-mc

Author: Jinyang He (MQ-mengqing)

Changes

The shouldInsertExtraNopBytesForCodeAlign() need STI to check whether relax is enabled or not. It is initialized when call setEmitNops. The setEmitNops may not be called in a section which has instructions but is not executable. In this case uninitialized STI will cause problems. Thus, check hasEmitNops before call it.

Fixes: #76552 (comment)


Full diff: https://github.com/llvm/llvm-project/pull/77236.diff

2 Files Affected:

  • (modified) llvm/lib/MC/MCExpr.cpp (+1-1)
  • (added) llvm/test/MC/RISCV/align-non-executable.s (+29)
diff --git a/llvm/lib/MC/MCExpr.cpp b/llvm/lib/MC/MCExpr.cpp
index 9dae026535ccfb..80def6dfc24b1a 100644
--- a/llvm/lib/MC/MCExpr.cpp
+++ b/llvm/lib/MC/MCExpr.cpp
@@ -708,7 +708,7 @@ static void AttemptToFoldSymbolOffsetDifference(
       if (DF) {
         Displacement += DF->getContents().size();
       } else if (auto *AF = dyn_cast<MCAlignFragment>(FI);
-                 AF && Layout &&
+                 AF && Layout && AF->hasEmitNops() &&
                  !Asm->getBackend().shouldInsertExtraNopBytesForCodeAlign(
                      *AF, Count)) {
         Displacement += Asm->computeFragmentSize(*Layout, *AF);
diff --git a/llvm/test/MC/RISCV/align-non-executable.s b/llvm/test/MC/RISCV/align-non-executable.s
new file mode 100644
index 00000000000000..a5bccf360729e1
--- /dev/null
+++ b/llvm/test/MC/RISCV/align-non-executable.s
@@ -0,0 +1,29 @@
+# RUN: llvm-mc --filetype=obj --triple=riscv64 --mattr=+relax %s \
+# RUN:     | llvm-readobj -r - | FileCheck --check-prefixes=CHECK,RELAX %s
+# RUN: llvm-mc --filetype=obj --triple=riscv64 --mattr=-relax %s \
+# RUN:     | llvm-readobj -r - | FileCheck %s
+
+.section ".dummy", "a"
+.L1:
+  call func
+.p2align 3
+.L2:
+.dword .L2 - .L1
+.word .L2 - .L1
+.half .L2 - .L1
+.byte .L2 - .L1
+
+# CHECK:       Relocations [
+# CHECK-NEXT:    Section ({{.*}}) .rela.dummy {
+# CHECK-NEXT:      0x0 R_RISCV_CALL_PLT func 0x0
+# RELAX-NEXT:      0x0 R_RISCV_RELAX - 0x0
+# CHECK-NEXT:      0x8 R_RISCV_ADD64 .L2 0x0
+# CHECK-NEXT:      0x8 R_RISCV_SUB64 .L1 0x0
+# CHECK-NEXT:      0x10 R_RISCV_ADD32 .L2 0x0
+# CHECK-NEXT:      0x10 R_RISCV_SUB32 .L1 0x0
+# CHECK-NEXT:      0x14 R_RISCV_ADD16 .L2 0x0
+# CHECK-NEXT:      0x14 R_RISCV_SUB16 .L1 0x0
+# CHECK-NEXT:      0x16 R_RISCV_ADD8 .L2 0x0
+# CHECK-NEXT:      0x16 R_RISCV_SUB8 .L1 0x0
+# CHECK-NEXT:    }
+# CHECK-NEXT:  ]

@MQ-mengqing
Copy link
Contributor Author

Add: @MaskRay @nathanchance @SixWeining
For LoongArch we not handle aligment directive to emit R_LARCH_ALIGN at present, so it not trigger fault. I'll add similar test in patch that handle aligment directive.

@SixWeining SixWeining merged commit 7b45c54 into llvm:main Jan 9, 2024
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
…rCodeAlign (llvm#77236)

The shouldInsertExtraNopBytesForCodeAlign() need STI to check whether
relax is enabled or not. It is initialized when call setEmitNops. The
setEmitNops may not be called in a section which has instructions but is
not executable. In this case uninitialized STI will cause problems.
Thus, check hasEmitNops before call it.

Fixes:
llvm#76552 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:mc Machine (object) code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants