Page MenuHomePhabricator

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

Authored by ro on Sep 14 2020, 7:55 AM.

Details

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.

Diff Detail

Event Timeline

ro created this revision.Sep 14 2020, 7:55 AM
ro requested review of this revision.Sep 14 2020, 7:55 AM

Would it be possible to add some tests?

ro added a comment.Sep 14 2020, 8:37 AM

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.

In D87615#2271297, @ro 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 requested changes to this revision.Sep 14 2020, 11:29 AM
joerg added a subscriber: joerg.

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

This revision now requires changes to proceed.Sep 14 2020, 11:29 AM
ro added a comment.Sep 15 2020, 1:27 AM

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 added a comment.Sep 15 2020, 1:35 AM

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 updated this revision to Diff 291831.Sep 15 2020, 1:42 AM
ro retitled this revision from [clang][Driver] Force stack realignment on 32-bit Solaris/x86 to [X86] Fix stack alignment on 32-bit Solaris/x86.

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
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2020, 1:42 AM
ro added a subscriber: vitalybuka.Sep 15 2020, 2:27 AM
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 updated this revision to Diff 291837.Sep 15 2020, 2:31 AM
ro added a reviewer: vitalybuka.

Allow for whitespace differences in vptr.cpp.

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

Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2020, 2:31 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
efriedma added inline comments.Sep 15 2020, 12:23 PM
llvm/lib/Target/X86/X86Subtarget.cpp
270

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

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 updated this revision to Diff 292153.Sep 16 2020, 4:00 AM
  • 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 added a comment.Sep 16 2020, 4:02 AM

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.

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

joerg accepted this revision.Sep 16 2020, 3:08 PM

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

This revision is now accepted and ready to land.Sep 16 2020, 3:08 PM
MaskRay accepted this revision.Sep 16 2020, 8:27 PM

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

This sentence from the description should be removed.

ro added a comment.Sep 17 2020, 2:08 AM

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 added a comment.Sep 17 2020, 2:10 AM

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.

This revision was automatically updated to reflect the committed changes.