In D118450#3630074, @ro wrote:

In D118450#3628904, @lhames wrote:

In D118450#3304545, @brad wrote:

In D118450#3304109, @ro wrote:

... which suggests that maybe HaveJIT should be set to false in SparcTargetInfo.cpp until this patch is resolved?

No, that something other than the tests uses the MCJIT support.

Lots of things use MCJIT, but MCJIT/Sparc can't have been be used anywhere, since this is the patch that introduces support.

In D120059#3338258, @ro wrote:

Abandoned unless absolutely necessary for Illumos support. As discussed, a way better alternative would be for them to implement dlpi_tls_modid or at least provide a way to distuingish Illumos from Solaris as outlined in Issue #53919.

The patch looks sensible, but isn't actually exercised: I ran a sparcv9-sun-solaris2.11 build with it included, which made no difference. However, on closer inspection, the sanitizer_coverage_trace_pc_guard*.cpp tests didn't run: they aren't supported together with ubsan, while asan isn't supported on SPARC due to a clang bug/limitation.

In D120472#3345035, @uweigand wrote:

These test now all seem to fail on s390x:
https://lab.llvm.org/buildbot/#/builders/94/builds/7962

LGTM

In D120381#3342008, @XiaodongLoong wrote:

In D120381#3342217, @brad wrote:

Looking at existing cases of comparisons with Triple::sparc and Triple::sparcv9, I guess we should have isSPARC32 and isSPARC64, too, like isMIPS32/isMIPS64 and isPPC32/isPPC64. When switching to isSPARC* from existing Triple::sparc* comparisons, one will have to be extra careful if they are meant to mean SPARC in general or 32-bit SPARC in particular. little-endian SPARC is another beast altogether: I didn't even know it existed beyond the ability of SPARCV9 CPUs to partially run in LE mode, so it will be hard to tell if LE SPARC is to be included. I guess only code review and testing by interested parties can tell.

• Whitespace fixes.
• Tighten error checking.

In D120362#3339616, @XiaodongLoong wrote:

• Formatting fixes.
• Improve/test error handling.

Comment and whitespace fixes.

Whitespace fixes.

Abandoned unless absolutely necessary for Illumos support. As discussed, a way better alternative would be for them to implement dlpi_tls_modid or at least provide a way to distuingish Illumos from Solaris as outlined in Issue #53919.

Superceded by D119829 and D120048. If absolutely necessary, D120059 could be revived if Illumos cannot implement dlpi_tls_modid.

• Split off MC and CodeGen parts as D120318.
• Add testcases.

I've finally gotten around to this: MC and CodeGen parts split off as D120318.

@MaskRay: With this patch effectively abandoned and the Illumos situation in their court, are you ok with D120048 going in now as is?

I've now updated Issue #46473 for this failure and posted D120238 to XFAIL the test. I think that's the best solution for now and in line with what was done for the other affected testcases. This way, when/if the bug gets fixed, all affected tests start to XPASS and can be adjusted.

Testcase formatting improvements.

Test fixes as per review comments.

In D120059#3331363, @MaskRay wrote:

In D120059#3330523, @MaskRay wrote:

In D120059#3330337, @MaskRay wrote:

D120048 decreases the number of lines and looks good to me. But for this one, there is now more complexity... Can we just let old systems (Solaris<=11.3) use *addr = 0; *size = 0;?

In D91605#3329375, @ro wrote:

In D91605#3322312, @ro wrote:

In D91605#3321554, @MaskRay wrote:

GetTls is about the static TLS block size. It consists of the main executable's TLS, and initially loaded shared objects' TLS, struct pthread, and padding.
Solaris should be able to just use the code path like FreeBSD, Linux musl, and various unpopular architectures of Linux glibc:

#elif SANITIZER_FREEBSD || SANITIZER_LINUX
  uptr align;
  GetStaticTlsBoundary(addr, size, &align);

Not unconditionally, unfortunally: as the comment above GetSizeFromHdr explains, dlpi_tls_modid was only introduced in an update to Solaris 11.4 FCS (which is sort of a problem), but isn't present in 11.3 (don't reallly care) and Illumos (this would break compilation for them). OTOH my solution is successfully being used in GCC's libphobos on Solaris 11.3, 11.4, and most likely Illumos, too. I'd rather not burn the Illumos bridge if it can be avoided.

In D119695#3322927, @joerg wrote:

Wouldn't the correct solution for Illumos be to use posix_madvise instead? Possibly with __EXTENSIONS__. Declaring prototypes like this is just begging for problems long term...

In D91605#3321556, @MaskRay wrote:

The Clang driver change should be in a separate patch with clang/test/Driver tests.

In D91605#3321554, @MaskRay wrote:

GetTls is about the static TLS block size. It consists of the main executable's TLS, and initially loaded shared objects' TLS, struct pthread, and padding.
Solaris should be able to just use the code path like FreeBSD, Linux musl, and various unpopular architectures of Linux glibc:

#elif SANITIZER_FREEBSD || SANITIZER_LINUX
  uptr align;
  GetStaticTlsBoundary(addr, size, &align);

In D119695#3320426, @MaskRay wrote:

I don't fully understand the caddr_t thing. If there is an action item for illumnos, we should make sure they know, so that llvm-project can drop the hacks.

@vitalybuka , @MaskRay could I persuade you to review the revision version which should be much less controversial and completely Solaris-specific? Thanks.

Since the build-time check for the ld -z relax=transtls option was met with massive resistance and was only necessary for Illumos anyway, this revision just uses it unconditionally.

LGTM

I just found the -disable-sparc-delay-filler option in llvm/lib/Target/Sparc/DelaySlotFiller.cpp. As expected, with it added to the llc invocation the unmodified test PASSes.

Simplify test.

Add tests in solaris-ld.c.

In D118021#3307079, @MaskRay wrote:

This needs some tests, otherwise there is a risk that others may break your changes when refactoring driver code.

Ping^2. It would be great if this could be reviewed/commited soon: it fixes hundreds of testsuite failures, so I'd like to get it into LLVM 14.

@eugenis: With D91615 now in, could you please commit this patch? It's the remaining piece required to make CompactRingBuffer.int64 work on Solaris/sparcv9.

In D95617#3305333, @dblaikie wrote:

In D118450#3299791, @brad wrote:

I noticed this bug report for PostgreSQL on Sparc for this particular functionality.

https://github.com/llvm/llvm-project/issues/47229

In D119119#3301984, @nickdesaulniers wrote:

Was the llc flag -stack-alignment=32 previously being ignored on sparc?

no-empty-child-vars.ll FAILs on SPARC: the first

; CHECK:     DW_TAG_subprogram
; CHECK:       DW_AT_abstract_origin (0x{{[0-9a-f]*}} "foo")

is missing completely and both qux and croix lack DW_TAG_inlined_subroutine.