Wrap whole file in if(LLVM_ENABLE_PIC).

Just found this had already been fixed in commit f0546173fa4bdde03ecb21a174fcaa8a6490adbd.

As described in D87825, this patch broke Debug builds on sparcv9-sun-solaris2.11 and amd64-pc-solaris2.11.

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.

In D87615#2278002, @joerg wrote:

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

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 ?

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

Allow for whitespace differences in vptr.cpp.

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

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

In D87615#2271646, @efriedma wrote:

In D87615#2271268, @RKSimon wrote:

Would it be possible to add some tests?

Ping? It's been a week and AFAICT there's nothing left for me to do.

Is there anything left to do to get approval? Thanks.

Add FIXME for SparcV8 MaxAtomicInlineWidth.

I've now discovered that not only the sparc assembler output requires GNU as, but the same is true for x86:

• as is invoked without -m64/-m32, thus chokes on amd64 insns since it default to 32-bit mode
• Even if adding -Wa,-m64, assembly fails:

Assembler: main.c
	"/var/tmp/main-faae1e.s", line 24 : Syntax error
	Near line: "	.section	".note.GNU-stack","",@progbits"

• There must be no double quotes around the section name.
• Without the quotes, the section name is no valid identifier: - isn't allowed.
• On top of that, Solaris doesn't care about those .note sections: to achieve non-executable stacks, you need e.g. ld -z sx=nxstack (Solaris 11.4) or ld -z nxstack (Solaris 11.3).

In D86621#2249240, @jrtc27 wrote:

Actually, __sparcv8 is only for V8; if you have 32-bit V9 on Solaris it defines __sparcv8plus _instead_:

jrtc27@gcc-solaris11:~$ /opt/solarisstudio12.4/bin/cc -E - -xarch=v9 -m32 -xdumpmacros </dev/null 2>&1 | grep sparc
#define __sparcv8plus 1
#define __sparc 1
#define sparc 1

Define __sparcv8 only on 32-bit Solaris/SPARC.
Update testcase.
Remove XFAILs that only apply to clang/2-stage builds.

In D86621#2247791, @jrtc27 wrote:

GCC on Linux defines __sparc_v9__ even with -m32. I don't know what Solaris does but please don't break other operating systems just because Solaris has broken headers that conflate the CPU and the ABI.

In D86621#2247786, @efriedma wrote:

Ping? It's been a week.

In D85225#2245621, @jyknight wrote:

ro: In this case, I would say to defer to Paul, although I wish that he would change his mind.

Actual Option 1 variant.

In D85225#2244825, @paulwalker-arm wrote:

My vote hasn't changed, I mean I already accepted the tweaked Option1 so you could have been done and dusted by now :) I just don't see what's gained from Option3's refactoring. It does the same as Option1 but in a less type safe/flexible/c++ way. Furthermore, the new interface is needlessly different to the other functions that handle SVE immediate values when the vector element type plays a role.

We've got 3 alternative versions now:

• my original one as simplified by Paul's suggestion (hadn't uploaded that, but certainly could recreate/retest if that's the selected candidate) Diff 1 in the History
• the sizeof version, Diff 3 in the History
• the (partial) non-template version, Diff 4 in the history

Non-template version, passing ImmBytes around.

In D85630#2241754, @hans wrote:

Feels like a hack, but I'm not strongly opposed either.

Handle both Release and RelWithDebInfo builds, downgrade from both -O3 and -O2 to
-O.

While doing 2-stage builds, the 2 XFAILs due to Bug 42493 were required. However, I've since also done a 1-stage build with gcc 10, which shows

Unexpectedly Passed Tests (2):
  UBSan-Standalone-sparc :: TestCases/Float/cast-overflow.cpp
  UBSan-Standalone-sparc :: TestCases/Misc/log-path_test.cpp

which is no wonder since the underlying bug only exists in clang long double handling.

In D85627#2239832, @Meinersbur wrote:

In D85627#2226214, @ro wrote:

In D85627#2206550, @Meinersbur wrote:

What do you mean by intended solution? My intent was to avoid breaking the build with PIC code turned off. Just as Windows doesn't support loadable modules and thus creation of LLVMPolly.so is disabled, it's just not possible to build a loadable module/shared object without PIC code, so there's no choice but to disable it.

I had a warning in mind, but you are right, there is no warning for Windows either. I didn't see the similarity. I could still be improved.

I've just completed a sparcv9-sun-solaris2.11 2-stage RelWithDebInfo build with your patch applied: worked fine. Thanks a lot for the quick fix.

This patch broke sparcv9-sun-solaris2.11 RelWithDebInfo build: cf. Bug 47303.

In D85630#2223304, @ro wrote:

In D85630#2221464, @tstellar wrote:

In D85630#2220694, @ro wrote:

Ping? It's been a week, the patch fixes ca. 250 testsuite failures and is this a candidate for LLVM 11.0.0 rc2.

You should file a bug and put 'release-11.0.0' in the blocker field to ensure this gets cherry-picked into the release/11.x branch.

Will do. However, by the time I posted the patch it wasn't (and still fully isn't) clear that the patch is ok/acceptable even for master.

In D86333#2235129, @vitalybuka wrote:

LGTM if it works. I don't have SunOS to test.

In D85116#2233800, @inferno wrote:

First, how did you verify that this patch is the culprit? While it seems plausible, I've seen too many cases where a likely candidate turned out to be innocent, wasting everyone's time.

When chromium folks were rolling clang, it was the only suspect in code coverage changes - llvmorg-12-init-1771-g1bd7046e : llvmorg-12-init-3492-ga1caa302 [like git hash, these are tags, git checkout to them work ]

In D85116#2231692, @inferno wrote:

We are seeing regressions on Linux, from this. See https://github.com/google/oss-fuzz/issues/4348. Can you please revert as it breaks coverage on several linux binaries.

In D85630#2228328, @ro wrote:

In D85630#2223313, @ro wrote:

In D85630#2223248, @hans wrote:

I agree with James.

Also, since you're applying this work-around for gcc, I assume the tests pass in the bootstrapped build, i.e. when compiling with clang?

I'd initially restricted the workaround to gcc because the reduced testcase attached to the GCC PR works when compiled with clang++ instead of g++. However, a bootstrap build is still nightmarishly bad on Solaris/sparcv9: the 11.0.0-rc1 one had 4817 failures, a fresh one on master with the workaround applied to both gcc and clang is down to 1365.

So there's at least some improvement, but still a long way to go.

Update comment.
Use -O with clang, too.

In D85225#2223849, @paulwalker-arm wrote:

I have no real objections to this patch other than I prefer:

bool IsByte = std::is_same<int8_t, std::make_signed_t<T>>::value ||
              std::is_same<int8_t, T>::value;

if that works for your use case, if not then I'm happy with your current solution. Please can you give others a day or so longer to intervene in case they have a better suggestion.

In D85627#2206550, @Meinersbur wrote:

Could you outline your intended solution? If I read correctly, with LLVM_ENABLE_PIC=OFF, no Polly module will be built, making Polly useless unless Polly is built in-tree and LLVM_POLLY_LINK_INTO_TOOLS is used.

Improve comments.

Tested on sparcv9-sun-solaris2.11 and amd64-pc-solaris2.11 (both Solaris 11.4 and OpenIndiana).

In D85415#2223611, @ro wrote:

In D85415#2216469, @efriedma wrote:

Instead of adapting the affected tests, this patch changes that default. The internal assembler still accepts both forms as input, only the output syntax is affected.

There's a check in the parser for usesSunStyleELFSectionSwitchSyntax(); what effect does that have?

It now rejects the Sun-style syntax on input. That's certainly not what I wanted to happen: instead my intent was to produce common ELF syntax on output, but accept both on input as the current/unpatched clang does. I must have been dreaming when I tested this!

In D85225#2224935, @MaskRay wrote:

The problem boils down to the fact that int8_t is plain char on Solaris: both the sparc and i386 psABIs have char as signed. However, with ...

So, is -funsigned-char unsupported on Solaris?

Use sizeof(T) == 2 check, too.
Update variable name.

Just check sizeof(T) == 1.

In D86166#2224690, @pcc wrote:

@ro On D85870 you mentioned that the declaration has been changed in Solaris 11.4 to use void * which causes a build break on 11.4 due to the incompatible declaration. I checked illumos and it looks like their declaration still has caddr_t, although the comment above this declaration implies that that shouldn't matter because the system's declaration isn't visible. But what about older versions of Solaris? Presumably they still have a declaration with caddr_t so I imagine the build would break with an incompatible declaration error there. Or do we not care about older versions?

In D85415#2216469, @efriedma wrote:

Instead of adapting the affected tests, this patch changes that default. The internal assembler still accepts both forms as input, only the output syntax is affected.

There's a check in the parser for usesSunStyleELFSectionSwitchSyntax(); what effect does that have?

Ping^2? Two weeks now.

In D85346#2223224, @vitalybuka wrote:

At least 3 of selected reviewer are not active recently.

In D85630#2223248, @hans wrote:

I agree with James.

Also, since you're applying this work-around for gcc, I assume the tests pass in the bootstrapped build, i.e. when compiling with clang?

In D85630#2221464, @tstellar wrote:

In D85630#2220694, @ro wrote:

Ping? It's been a week, the patch fixes ca. 250 testsuite failures and is this a candidate for LLVM 11.0.0 rc2.

You should file a bug and put 'release-11.0.0' in the blocker field to ensure this gets cherry-picked into the release/11.x branch.

In D85630#2222583, @jyknight wrote:

Presumably this is a bug in something -- either the solaris linker, in g++, or in the llvm code being mis-compiled? It seems unfortunate to put in place a hacky workaround like this, without a bug reference to the responsible component.

Ping? Two weeks now.

Ping? It's been a week, the patch fixes ca. 250 testsuite failures and is this a candidate for LLVM 11.0.0 rc2.