[PATCH] D87615: [clang][Driver] Force stack realignment on 32-bit Solaris/x86

[Rainer Orth via Phabricator via cfe-commits]

ro added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87615/new/

https://reviews.llvm.org/D87615

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87615: [clang][Driver] Force stack realignment on 32-bit Solaris/x86

[Rainer Orth via Phabricator via cfe-commits]

ro added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87615/new/

https://reviews.llvm.org/D87615

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87615: [clang][Driver] Force stack realignment on 32-bit Solaris/x86

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg requested changes to this revision.
joerg added a comment.
This revision now requires changes to proceed.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87615/new/

https://reviews.llvm.org/D87615

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87615: [clang][Driver] Force stack realignment on 32-bit Solaris/x86

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87615/new/

https://reviews.llvm.org/D87615

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87615: [clang][Driver] Force stack realignment on 32-bit Solaris/x86

[Rainer Orth via Phabricator via cfe-commits]

ro added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87615/new/

https://reviews.llvm.org/D87615

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87615: [clang][Driver] Force stack realignment on 32-bit Solaris/x86

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added a comment.

Would it be possible to add some tests?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87615/new/

https://reviews.llvm.org/D87615

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87615: [clang][Driver] Force stack realignment on 32-bit Solaris/x86

[Rainer Orth via Phabricator via cfe-commits]

ro created this revision.
ro added reviewers: MaskRay, rahmanl, efriedma, RKSimon.
ro added a project: clang.
Herald added a subscriber: fedor.sergeev.
ro requested review of this revision.

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`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87615

Files:
  clang/lib/Driver/ToolChains/Clang.cpp


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2030,6 +2030,14 @@
   const Driver  = getToolChain().getDriver();
   addX86AlignBranchArgs(D, Args, CmdArgs, /*IsLTO=*/false);
 
+  // 32-bit Solaris/x86 only guarantees 4-byte stack alignment as required by
+  // the i386 psABI, so realign it as necessary for SSE instructions.
+  const llvm::Triple  = getToolChain().getTriple();
+  if (Triple.isOSSolaris() && Triple.getArch() == llvm::Triple::x86 &&
+  Args.hasFlag(options::OPT_mstackrealign, options::OPT_mno_stackrealign,
+   true))
+CmdArgs.push_back("-mstackrealign");
+
   if (!Args.hasFlag(options::OPT_mred_zone, options::OPT_mno_red_zone, true) ||
   Args.hasArg(options::OPT_mkernel) ||
   Args.hasArg(options::OPT_fapple_kext))


Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2030,6 +2030,14 @@
   const Driver  = getToolChain().getDriver();
   addX86AlignBranchArgs(D, Args, CmdArgs, /*IsLTO=*/false);
 
+  // 32-bit Solaris/x86 only guarantees 4-byte stack alignment as required by
+  // the i386 psABI, so realign it as necessary for SSE instructions.
+  const llvm::Triple  = getToolChain().getTriple();
+  if (Triple.isOSSolaris() && Triple.getArch() == llvm::Triple::x86 &&
+  Args.hasFlag(options::OPT_mstackrealign, options::OPT_mno_stackrealign,
+   true))
+CmdArgs.push_back("-mstackrealign");
+
   if (!Args.hasFlag(options::OPT_mred_zone, options::OPT_mno_red_zone, true) ||
   Args.hasArg(options::OPT_mkernel) ||
   Args.hasArg(options::OPT_fapple_kext))
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits