[X86] Fix stack alignment on 32-bit Solaris/x86

[ro]

Summary

On Solaris/x86, several hundred 32-bit tests FAIL, all in the same way:

env ASAN_OPTIONS=halt_on_error=false ./halt_on_error_suppress_equal_pcs.cpp.tmp
Segmentation Fault (core dumped)

They segfault during startup:

Thread 2 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 1 (LWP 1)]
0x080f21f0 in __sanitizer::internal_mmap(void*, unsigned long, int, int, int, unsigned long long) () at /vol/llvm/src/llvm-project/dist/compiler-rt/lib/sanitizer_common/sanitizer_solaris.cpp:65
65	                             int prot, int flags, int fd, OFF_T offset) {
1: x/i $pc
=> 0x80f21f0 <_ZN11__sanitizer13internal_mmapEPvmiiiy+16>:	movaps 0x30(%esp),%xmm0
(gdb) p/x $esp
$3 = 0xfeffd488

The problem is that movaps expects 16-byte alignment, while 32-bit Solaris/x86
only guarantees 4-byte alignment following the i386 psABI.

This patch avoid the issue by defaulting to -mstackrealign, just like gcc.

Tested on amd64-pc-solaris2.11.

[RKSimon]

Would it be possible to add some tests?

[ro]

In D87615#2271268, @RKSimon wrote:

Would it be possible to add some tests?

Probably not reliably: the tests that usually fail bye the hundreds happen to PASS once in a while when the stack pointer is 16-byte aligned
by accident. Otherwise, the failure is prominent enough, I'd think.

[efriedma]

In D87615#2271297, @ro wrote:

In D87615#2271268, @RKSimon wrote:

Would it be possible to add some tests?

Probably not reliably: the tests that usually fail bye the hundreds happen to PASS once in a while when the stack pointer is 16-byte aligned
by accident. Otherwise, the failure is prominent enough, I'd think.

Tests that check we realign the stack, not tests for the crash itself.

---

X86Subtarget::initSubtargetFeatures claims that that stack is 16-byte aligned on Solaris; if that's wrong, we should fix it, not force stack realignment.

[joerg]

I don't think this is the right place for this at all. Look at X86Subtarget::initSubtargetFeatures please.

[ro]

In D87615#2271646, @efriedma wrote:

X86Subtarget::initSubtargetFeatures claims that that stack is 16-byte aligned on Solaris; if that's wrong, we should fix it, not force stack realignment.

I missed that (my usual issue of config info spread over llvm and clang). I'll shortly post an alternative patch using that approach.

[ro]

That claim of 16-byte alignment on Solaris is half-wrong: it's definitely wrong on Solaris, but seems to be true on Illumos.

However, there's currently no way to distinguish the two other than checking uname -v at runtime in a native compiler. The configure triples continue to be identical. Maybe it's possible to do the uname check in llvm/lib/Support/Unix/Host.inc (updateTripleOSVersion) changing the triple to (say) x86_64-pc-illumos at runtime. Then isOSSolaris would return true for both Solaris and Illumos, while a new isOSIllumos would only return true on Illumos: I think such a subtarget approach is what they used in Go. However, this might well break elsewhere and it's ultimately up to the Illumos community to decide how to deal with the Solaris/Illumos differences.

[ro]

Set stackAlignment instead.

Tested on amd64-pc-solaris2.11. However, compared to the -mstackrealign version
there's one regression that I still need to investigate:

UBSan-Standalone-i386 :: TestCases/TypeCheck/vptr.cpp

[ro]

In D87615#2273427, @ro wrote:

Tested on amd64-pc-solaris2.11. However, compared to the -mstackrealign version
there's one regression that I still need to investigate:

UBSan-Standalone-i386 :: TestCases/TypeCheck/vptr.cpp

This turns out to be just a whitespace difference: before the stack alignment patch I'd get for the mS subtest:

Test case: mS
/vol/llvm/src/llvm-project/dist/compiler-rt/test/ubsan/TestCases/TypeCheck/vptr.cpp:169:15: runtime error: member access within address 0xfeffdcf0 which does not point to an object of type 'T'
0xfeffdcf0: note: object is of type 'S'
 76 62 92 92  ec 1b 06 08 00 00 00 00  44 de ff fe 30 dd ff fe  00 00 00 00 00 00 00 00  00 00 00 00
              ^~~~~~~~~~~
              vptr for 'S'

After the patch I see instead:

Test case: mS
/vol/llvm/src/llvm-project/dist/compiler-rt/test/ubsan/TestCases/TypeCheck/vptr.cpp:169:15: runtime error: member access within address 0xfeffdd34 which does not point to an object of type 'T'
0xfeffdd34: note: object is of type 'S'
  76 62 92 92 ec 1b 06 08  00 00 00 00 78 dd ff fe  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
              ^~~~~~~~~~~
              vptr for 'S'

i.e. one additional blank at the beginning of the line and a different grouping. I have no idea about the significance of those: @vitalybuka?

[ro]

Allow for whitespace differences in vptr.cpp.

Tested on amd64-pc-solaris2.11 with the previous -mstackrealign patch and with the new stackAlignment one.

[efriedma]

llvm/lib/Target/X86/X86Subtarget.cpp

270

stackAlignment is initialized to 4 in the header, so stackAlignment = Align(4) here is a no-op.

[efriedma]

Also, it would be nice to have some regression test coverage; add a Solaris RUN line to llvm/test/CodeGen/X86/stack-align2.ll ?

[ro]

• Rely on stackAlignment default for 32-bit Solaris/x86
• Handle Solaris in llvm/test/CodeGen/X86/stack-align2.ll

Tested on amd64-pc-solaris2.11 and x86_64-pc-linux-gnu.

[ro]

In D87615#2275045, @efriedma wrote:

Also, it would be nice to have some regression test coverage; add a Solaris RUN line to llvm/test/CodeGen/X86/stack-align2.ll ?

Sure, done in the updated patch. I'd checked that if FAILs with llc -mtriple=i386-pc-solaris2.11 --stack-alignment=16

llvm/lib/Target/X86/X86Subtarget.cpp

270

Ah, I missed that. Fixed in the updated patch. Thanks.

[efriedma]

LGTM

[RKSimon]

@joerg Are you alright with this now? If so please can you accept it to unblock it.

[joerg]

I'm still curious about the source of the vptr diff, but that's a minor question, otherwise. LGTM

[MaskRay]

This patch avoid the issue by defaulting to -mstackrealign, just like gcc.

This sentence from the description should be removed.

[ro]

In D87615#2278002, @joerg wrote:

I'm still curious about the source of the vptr diff, but that's a minor question, otherwise. LGTM

Thanks. I think I've found what's going on here: the memory ranges are ultimately printed by compiler-rt/lib/ubsan/ubsan_diag.cpp (PrintMemorySnippet). The crucial snippet is around l.280:

// Emit data.
InternalScopedString Buffer(1024);
for (uptr P = Min; P != Max; ++P) {
  unsigned char C = *reinterpret_cast<const unsigned char*>(P);
  Buffer.append("%s%02x", (P % 8 == 0) ? "  " : " ", C);
}

Min is Loc - 4, i.e. 4 bytes before the start location. Before my patch (i.e. with 16-byte stack alignment), that was

0xfeffdcf0: note: object is of type 'S'
 76 62 92 92  ec 1b 06 08 00 00 00 00  44 de ff fe 30 dd ff fe  00 00 00 00 00 00 00 00  00 00 00 00

i.e. Loc - 4 wasn't on an 8-byte boundary, thus the line starts with a single blank. With 4-byte aligned stack, I have instead

0xfeffdd34: note: object is of type 'S'
  76 62 92 92 ec 1b 06 08  00 00 00 00 78 dd ff fe  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00

i.e. Loc - 4 is on an 8-byte boundary and the line starts with two blanks.

I can't see that the vptr.cpp testcase does anything to guarantee a specific alignment here, or depends on one. In fact, AFAICS it's the only ubsan test that looks at that memory dump line at all.

[ro]

In D87615#2278436, @MaskRay wrote:

This patch avoid the issue by defaulting to -mstackrealign, just like gcc.

This sentence from the description should be removed.

Sure: I usually review and eventually update the description once for the commit message, rather than constantly tinkering with it while the review is still ongoing.

compiler-rt/test/ubsan/TestCases/TypeCheck/vptr.cpp

for (int i = 0; i < 2; i++) {
       // CHECK-LOC-SUPPRESS-NEXT: {{^              vptr for 'V'}}
       p->g();
     }
     return 0;
 
   case 'm':
     // CHECK-MEMBER: vptr.cpp:[[@LINE+6]]:15: runtime error: member access within address [[PTR:0x[0-9a-f]*]] which does not point to an object of type 'T'
     // CHECK-MEMBER-NEXT: [[PTR]]: note: object is of type [[DYN_TYPE:'S'|'U']]
-    // CHECK-MEMBER-NEXT: {{^ .. .. .. ..  .. .. .. .. .. .. .. ..  }}
+    // CHECK-MEMBER-NEXT: {{^  ?.. .. .. ..  ?.. .. .. ..  ?.. .. .. ..  ?}}
     // CHECK-MEMBER-NEXT: {{^              \^~~~~~~~~~~(~~~~~~~~~~~~)? *$}}
     // CHECK-MEMBER-NEXT: {{^              vptr for}} [[DYN_TYPE]]
     // CHECK-Linux-MEMBER: #0 {{.*}}access_p{{.*}}vptr.cpp:[[@LINE+1]]
     return p->b;
 
     // CHECK-INVALID-MEMBER: vptr.cpp:[[@LINE-2]]:15: runtime error: member access within address [[PTR:0x[0-9a-f]*]] which does not point to an object of type 'T'
     // CHECK-INVALID-MEMBER-NEXT: [[PTR]]: note: object has invalid vptr
     // CHECK-INVALID-MEMBER-NEXT: {{^  ?.. .. .. ..  ?00 00 00 00  ?00 00 00 00  ?}}
     // CHECK-INVALID-MEMBER-NEXT: {{^              \^~~~~~~~~~~(~~~~~~~~~~~~)? *$}}
     // CHECK-INVALID-MEMBER-NEXT: {{^              invalid vptr}}
     // CHECK-Linux-NULL-MEMBER: #0 {{.*}}access_p{{.*}}vptr.cpp:[[@LINE-7]]
 
   case 'f':
     // CHECK-MEMFUN: vptr.cpp:[[@LINE+6]]:15: runtime error: member call on address [[PTR:0x[0-9a-f]*]] which does not point to an object of type 'T'
     // CHECK-MEMFUN-NEXT: [[PTR]]: note: object is of type [[DYN_TYPE:'S'|'U']]
-    // CHECK-MEMFUN-NEXT: {{^ .. .. .. ..  .. .. .. .. .. .. .. ..  }}
+    // CHECK-MEMFUN-NEXT: {{^  ?.. .. .. ..  ?.. .. .. ..  ?.. .. .. ..  ?}}
     // CHECK-MEMFUN-NEXT: {{^              \^~~~~~~~~~~(~~~~~~~~~~~~)? *$}}
     // CHECK-MEMFUN-NEXT: {{^              vptr for}} [[DYN_TYPE]]
     // TODO: Add check for stacktrace here.
     return p->g();
 
   case 'o':
     // CHECK-OFFSET: vptr.cpp:[[@LINE+6]]:37: runtime error: member call on address [[PTR:0x[0-9a-f]*]] which does not point to an object of type 'U'
     // CHECK-OFFSET-NEXT: 0x{{[0-9a-f]*}}: note: object is base class subobject at offset {{8|16}} within object of type [[DYN_TYPE:'U']]
     // CHECK-OFFSET-NEXT: {{^ .. .. .. ..  .. .. .. .. .. .. .. ..  .. .. .. .. .. .. .. ..  .. .. .. .. .. .. .. ..  }}
     // CHECK-OFFSET-NEXT: {{^              \^                        (                         ~~~~~~~~~~~~)?~~~~~~~~~~~ *$}}
     // CHECK-OFFSET-NEXT: {{^                                       (                         )?vptr for}} 'T' base class of [[DYN_TYPE]]
     // CHECK-Linux-OFFSET: #0 {{.*}}access_p{{.*}}vptr.cpp:[[@LINE+1]]
     return reinterpret_cast<U*>(p)->v() - 2;
 
   case 'c':
     // CHECK-DOWNCAST: vptr.cpp:[[@LINE+6]]:11: runtime error: downcast of address [[PTR:0x[0-9a-f]*]] which does not point to an object of type 'T'
     // CHECK-DOWNCAST-NEXT: [[PTR]]: note: object is of type [[DYN_TYPE:'S'|'U']]
-    // CHECK-DOWNCAST-NEXT: {{^ .. .. .. ..  .. .. .. .. .. .. .. ..  }}
+    // CHECK-DOWNCAST-NEXT: {{^  ?.. .. .. ..  ?.. .. .. ..  ?.. .. .. ..  ?}}
     // CHECK-DOWNCAST-NEXT: {{^              \^~~~~~~~~~~(~~~~~~~~~~~~)? *$}}
     // CHECK-DOWNCAST-NEXT: {{^              vptr for}} [[DYN_TYPE]]
     // CHECK-Linux-DOWNCAST: #0 {{.*}}access_p{{.*}}vptr.cpp:[[@LINE+1]]
     (void)static_cast<T*>(reinterpret_cast<S*>(p));
     return 0;
 
   case 'n':
     // CHECK-NULL-MEMFUN: vptr.cpp:[[@LINE+1]]:15: runtime error: member call on null pointer of type 'T'

llvm/lib/Target/X86/X86Subtarget.cpp

void X86Subtarget::initSubtargetFeatures(StringRef CPU, StringRef TuneCPU,
 
   LLVM_DEBUG(dbgs() << "Subtarget features: SSELevel " << X86SSELevel
                     << ", 3DNowLevel " << X863DNowLevel << ", 64bit "
                     << HasX86_64 << "\n");
   if (In64BitMode && !HasX86_64)
     report_fatal_error("64-bit code requested on a subtarget that doesn't "
                        "support it!");
 
-  // Stack alignment is 16 bytes on Darwin, Linux, kFreeBSD and Solaris (both
-  // 32 and 64 bit) and for all 64-bit targets.
+  // Stack alignment is 16 bytes on Darwin, Linux, kFreeBSD and for all
+  // 64-bit targets.  On Solaris (32-bit), stack alignment is 4 bytes
+  // following the i386 psABI, while on Illumos it is always 16 bytes.
   if (StackAlignOverride)
     stackAlignment = *StackAlignOverride;
-  else if (isTargetDarwin() || isTargetLinux() || isTargetSolaris() ||
-           isTargetKFreeBSD() || In64BitMode)
+  else if (isTargetDarwin() || isTargetLinux() || isTargetKFreeBSD() ||
+           In64BitMode)
     stackAlignment = Align(16);
 
   // Consume the vector width attribute or apply any target specific limit.
   if (PreferVectorWidthOverride)
     PreferVectorWidth = PreferVectorWidthOverride;
   else if (Prefer128Bit)
     PreferVectorWidth = 128;
   else if (Prefer256Bit)
     PreferVectorWidth = 256;
 }
 

llvm/test/CodeGen/X86/stack-align2.ll

 ; RUN: llc < %s -mcpu=generic -mtriple=i386-linux | FileCheck %s -check-prefix=LINUX-I386
 ; RUN: llc < %s -mcpu=generic -mtriple=i386-kfreebsd | FileCheck %s -check-prefix=KFREEBSD-I386
 ; RUN: llc < %s -mcpu=generic -mtriple=i386-netbsd | FileCheck %s -check-prefix=NETBSD-I386
 ; RUN: llc < %s -mcpu=generic -mtriple=i686-apple-darwin8 | FileCheck %s -check-prefix=DARWIN-I386
+; RUN: llc < %s -mcpu=generic -mtriple=i386-pc-solaris2.11 | FileCheck %s -check-prefix=SOLARIS-I386
 ; RUN: llc < %s -mcpu=generic -mtriple=x86_64-linux | FileCheck %s -check-prefix=LINUX-X86_64
 ; RUN: llc < %s -mcpu=generic -mtriple=x86_64-kfreebsd | FileCheck %s -check-prefix=KFREEBSD-X86_64
 ; RUN: llc < %s -mcpu=generic -mtriple=x86_64-netbsd | FileCheck %s -check-prefix=NETBSD-X86_64
 ; RUN: llc < %s -mcpu=generic -mtriple=x86_64-apple-darwin8 | FileCheck %s -check-prefix=DARWIN-X86_64
+; RUN: llc < %s -mcpu=generic -mtriple=x86_64-pc-solaris2.11 | FileCheck %s -check-prefix=SOLARIS-X86_64
 
 define i32 @test() nounwind {
 entry:
   call void @test2()
   ret i32 0
 
 ; LINUX-I386:     subl  $12, %esp
 ; KFREEBSD-I386:  subl  $12, %esp
 ; DARWIN-I386:    subl  $12, %esp
-; NETBSD-I386-NOT: subl {{.*}}, %esp
+; NETBSD-I386-NOT:  subl  {{.*}}, %esp
+; SOLARIS-I386-NOT: subl  {{.*}}, %esp
 
 ; LINUX-X86_64:      pushq %{{.*}}
 ; LINUX-X86_64-NOT:  subq {{.*}}, %rsp
 ; DARWIN-X86_64:     pushq %{{.*}}
 ; DARWIN-X86_64-NOT: subq {{.*}}, %rsp
 ; NETBSD-X86_64:     pushq %{{.*}}
 ; NETBSD-X86_64-NOT: subq {{.*}}, %rsp
+; SOLARIS-X86_64:     pushq %{{.*}}
+; SOLARIS-X86_64-NOT: subq  {{.*}}, %rsp
 ; KFREEBSD-X86_64:     pushq %{{.*}}
 ; KFREEBSD-X86_64-NOT: subq {{.*}}, %rsp
 }
 
 declare void @test2()