Warn on align directive with non-zero fill value in virtual sections

[luismarques]

This patch warns when an align directive with a non-zero fill value is used in a virtual section. The fill value is also set to zero, preventing an assertion in MCAssembler::writeSectionData for the case
of MCFragment::FT_Align from tripping.

[llvmbot]

@llvm/pr-subscribers-mc

Changes

This patch warns when an align directive with a non-zero fill value is
used in a virtual section. The fill value is also set to zero,
preventing an assertion in MCAssembler::writeSectionData for the case
of MCFragment::FT_Align from tripping.

---

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

2 Files Affected:

• (modified) llvm/lib/MC/MCParser/AsmParser.cpp (+13-1)
• (modified) llvm/test/MC/ELF/nobits-non-zero-value.s (+3)

diff --git a/llvm/lib/MC/MCParser/AsmParser.cpp b/llvm/lib/MC/MCParser/AsmParser.cpp
index f70620c15a7ebb7..15ba96b84fa4701 100644
--- a/llvm/lib/MC/MCParser/AsmParser.cpp
+++ b/llvm/lib/MC/MCParser/AsmParser.cpp
@@ -3392,6 +3392,7 @@ bool AsmParser::parseDirectiveAlign(bool IsPow2, unsigned ValueSize) {
   bool HasFillExpr = false;
   int64_t FillExpr = 0;
   int64_t MaxBytesToFill = 0;
+  SMLoc FillExprLoc;
 
   auto parseAlign = [&]() -> bool {
     if (parseAbsoluteExpression(Alignment))
@@ -3402,7 +3403,7 @@ bool AsmParser::parseDirectiveAlign(bool IsPow2, unsigned ValueSize) {
       //  .align 3,,4
       if (getTok().isNot(AsmToken::Comma)) {
         HasFillExpr = true;
-        if (parseAbsoluteExpression(FillExpr))
+        if (parseTokenLoc(FillExprLoc) || parseAbsoluteExpression(FillExpr))
           return true;
       }
       if (parseOptionalToken(AsmToken::Comma))
@@ -3451,6 +3452,17 @@ bool AsmParser::parseDirectiveAlign(bool IsPow2, unsigned ValueSize) {
     }
   }
 
+  if (HasFillExpr) {
+    MCSection *Sec = getStreamer().getCurrentSectionOnly();
+    if (Sec && Sec->isVirtualSection()) {
+      ReturnVal |=
+          Warning(FillExprLoc, "ignoring non-zero fill value in " +
+                                   Sec->getVirtualSectionKind() + " section '" +
+                                   Sec->getName() + "'");
+      FillExpr = 0;
+    }
+  }
+
   // Diagnose non-sensical max bytes to align.
   if (MaxBytesLoc.isValid()) {
     if (MaxBytesToFill < 1) {
diff --git a/llvm/test/MC/ELF/nobits-non-zero-value.s b/llvm/test/MC/ELF/nobits-non-zero-value.s
index 16d386dc62ad549..8f37a957b6b56c0 100644
--- a/llvm/test/MC/ELF/nobits-non-zero-value.s
+++ b/llvm/test/MC/ELF/nobits-non-zero-value.s
@@ -12,5 +12,8 @@
 # CHECK: {{.*}}.s:[[#@LINE+1]]:3: error: SHT_NOBITS section '.bss' cannot have instructions
   addb %al,(%rax)
 
+# CHECK: {{.*}}.s:[[#@LINE+1]]:11: warning: ignoring non-zero fill value in SHT_NOBITS section '.bss'
+.align 4, 42
+
 # CHECK: <unknown>:0: error: SHT_NOBITS section '.bss' cannot have non-zero initializers
   .long 1

[s-barannikov]

Add check lines for --no-warn / --fatal-warnings?

[luismarques]

Add check lines for --no-warn / --fatal-warnings?

Can you walk me through why we might want that, please? I'd have guessed that if we properly emit a warning and the diagnostic infrastructure is working as intended then we wouldn't need to test such interactions for all kinds of warnings.

[s-barannikov]

Warning() returns true (indicating an error) only in presense of --fatal-warnings. We might want to add the --fatal-warnings check to test that we didn't forget ReturnVal |= part. (However, it appears that omitting it does not change the behavior because top-level loop not only checks for the return value, but also for what hasPendingErrors() returns.)
Regarding --no-warn, I tend to agree with you.

[luismarques]

I added a RUN with --fatal-warnings. Let me know if it addresses your concerns properly.

[s-barannikov]

This check line would match if Warning was replaced with Error. I expected something like:

# RUN: not llvm-mc ... | FileCheck %s
# RUN: not llvm-mc --fatal-warnings ... | FileCheck --check-prefix=FATAL %s
...
# CHECK: ... warning: ignoring ...
# FATAL: ... error: ignoring...

I grepped mc tests and found only 4 tests that excercise --fatal-warnings, so I guess you can just drop the last commit and go ahead...

[s-barannikov]

This check line would match if Warning was replaced with Error. I expected something like:

# RUN: not llvm-mc ... | FileCheck %s
# RUN: not llvm-mc --fatal-warnings ... | FileCheck --check-prefix=FATAL %s
...
# CHECK: ... warning: ignoring ...
# FATAL: ... error: ignoring...

I grepped mc tests and found only 4 tests that excercise --fatal-warnings, so I guess you can just drop the last commit and go ahead...

[MatzeB]

Hey this seems to produce a lot of invalid warnings for me. Example on a linux x86-64 system:

$ cat hello.cpp
template<typename T> struct C { static T comdat_global; };
template<typename T> T C<T>::comdat_global;
void* use = (void*)&C<int>::comdat_global;

Results in:

$ clang -S hello.cpp && clang -Wall -c hello.s
hello.s:6:14: warning: ignoring non-zero fill value in SHT_NOBITS section '.bss._ZN1CIiE13comdat_globalE'
        .p2align        2, 0x0

it seems this just checks for the presence of fill-expression but not if it is actually non-zero...

[MatzeB]

my comment was based on LLVM f548d19

[luismarques]

@MatzeB You're right, this patch was flawed. My original patch also checked the non-zero fill value but, for whatever reason, I seem to have removed that before I submitted the PR. Apologies. Here's a fix: #67237. @s-barannikov