[Sparc] Don't define __sparcv9 and __sparcv9__ when targeting V8+

[glaubitz]

Summary

Currently, clang defines the three macros sparcv9, sparcv9__
and sparc_v9 when targeting the V8+ baseline, i.e. using the
V9 instruction set on a 32-bit target.

Since neither gcc nor SolarisStudio define sparcv9 and sparcv9__
when targeting V8+, some existing code such as the glibc breaks when
defining either of these two macros on a 32-bit target as they are
used to detect a 64-bit target. Update the tests accordingly.

Fixes PR49562.

[brad]

I do not immediately see why the other tests are failing, but at a bare minimum the following test from clang/test/Preprocessor/predefined-arch-macros.c will have to be updated..

// RUN: %clang -mcpu=v9 -E -dM %s -o - 2>&1 \
// RUN:     -target sparc-unknown-linux \
// RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_SPARC-V9
// CHECK_SPARC-V9-NOT: #define __sparcv8 1
// CHECK_SPARC-V9-NOT: #define __sparcv8__ 1
// CHECK_SPARC-V9: #define __sparc_v9__ 1
// CHECK_SPARC-V9: #define __sparcv9 1
// CHECK_SPARC-V9: #define __sparcv9__ 1

[jrtc27]

clang/lib/Basic/Targets/Sparc.cpp

160–164

This is wrong, they should never be defined on any OS for 32-bit SPARC; see https://github.com/gcc-mirror/gcc/blob/master/gcc/config/sparc/netbsd-elf.h#L22 for example (FreeBSD only ever had a 64-bit port so isn't interesting to look at). That is, the contents of this if should just be deleted, leaving just __sparc_v9__.

244–247

This doesn't need changing, we can define more things than GCC to keep it simple.

[glaubitz]

clang/lib/Basic/Targets/Sparc.cpp

160–164

Agreed although I haven't verified whether `sparc_v9`` is defined for NetBSD with `-m32 -mcpu=v9``.

244–247

Well, my original intent was to match GCC to make sure we're 100% compatible and I would like to keep it that way.

[glaubitz]

In D98574#2624558, @brad wrote:

I do not immediately see why the other tests are failing, but at a bare minimum the following test from clang/test/Preprocessor/predefined-arch-macros.c will have to be updated..

// RUN: %clang -mcpu=v9 -E -dM %s -o - 2>&1 \
// RUN:     -target sparc-unknown-linux \
// RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_SPARC-V9
// CHECK_SPARC-V9-NOT: #define __sparcv8 1
// CHECK_SPARC-V9-NOT: #define __sparcv8__ 1
// CHECK_SPARC-V9: #define __sparc_v9__ 1
// CHECK_SPARC-V9: #define __sparcv9 1
// CHECK_SPARC-V9: #define __sparcv9__ 1

Thanks, I agree these need to be adjusted.

[glaubitz]

For reference, here is what GCC defines on Linux with regards to SPARC:

glaubitz@gcc202:~$ echo | gcc -E -dM -mcpu=v9 -m32 - |grep arch
glaubitz@gcc202:~$ echo | gcc -E -dM -mcpu=v9 -m32 - |grep LP
glaubitz@gcc202:~$ echo | gcc -E -dM -mcpu=v9 -m32 - |grep v9
#define __sparc_v9__ 1
glaubitz@gcc202:~$ echo | gcc -E -dM -mcpu=v9 -m64 - |grep arch
#define __arch64__ 1
glaubitz@gcc202:~$ echo | gcc -E -dM -mcpu=v9 -m64 - |grep LP
#define __LP64__ 1
#define _LP64 1
glaubitz@gcc202:~$ echo | gcc -E -dM -mcpu=v9 -m64 - |grep v9
#define __sparc_v9__ 1
glaubitz@gcc202:~$

And here is what it defines on OpenBSD:

openbsd# echo | gcc -E -dM -mcpu=v9 -m64 - |grep arch 
#define __arch64__ 1
openbsd# echo | gcc -E -dM -mcpu=v9 -m64 - |grep LP   
#define __LP64__ 1
#define _LP64 1
openbsd# echo | gcc -E -dM -mcpu=v9 -m64 - |grep sparc
#define sparc 1
#define __sparc__ 1
#define __sparc 1
#define __sparc64__ 1
#define __sparcv9__ 1
#define __sparc_v9__ 1
openbsd#

-m32 is not supported on OpenBSD at all:

openbsd# echo | gcc -E -dM -m32 -            
:0: error: -m32 is not supported by this configuration
openbsd#

For NetBSD, we have:

$ echo | gcc -E -dM -mcpu=v9 -m32 - |grep sparc
#define sparc 1
#define __sparc__ 1
#define __sparc 1
#define __sparc_v9__ 1
$ echo | gcc -E -dM -mcpu=v9 -m64 - |grep sparc
#define sparc 1
#define __sparc__ 1
#define __sparcv9 1
#define __sparc 1
#define __sparc64__ 1
#define __sparc_v9__ 1
$ echo | gcc -E -dM -mcpu=v9 -m32 - |grep LP
$ echo | gcc -E -dM -mcpu=v9 -m64 - | grep LP
#define __LP64__ 1
#define _LP64 1
$ echo | gcc -E -dM -mcpu=v9 -m32 - | grep arch
$ echo | gcc -E -dM -mcpu=v9 -m64 - | grep arch
#define __arch64__ 1

And with the native 32-bit compiler on NetBSD, we get:

$ echo | gcc -E -dM -mcpu=v9  - | grep sparc
#define sparc 1
#define __sparc__ 1
#define __sparc 1
#define __sparc_v9__ 1
$ echo | gcc -E -dM -mcpu=v9 - | grep LP
$ echo | gcc -E -dM -mcpu=v9 - | grep arch

So, I think clang behaves correctly with the changes in this diff - although it does not account for the difference between OpenBSD and NetBSD.

[brad]

In D98574#2641697, @glaubitz wrote:

-m32 is not supported on OpenBSD at all:

openbsd# echo | gcc -E -dM -m32 -            
:0: error: -m32 is not supported by this configuration
openbsd#

Yes. OpenBSD has no 32-bit support for sparc64 (nor does FreeBSD).

[glaubitz]

I have updated the patch now with the result that clang should behave as GCC now on Linux, NetBSD and OpenBSD.

[glaubitz]

Updated as I previously forgot to account for FreeBSD as well.

[ro]

clang/lib/Basic/Targets/Sparc.cpp

244–247

I agree with Jessica here: you're creating a complicated maze for no real gain. Besides, have you checked what gcc on the BSDs really does? They often neglect to get their changes upstream and what's in the gcc repo doesn't necessarily represent what they actually use.

[glaubitz]

clang/lib/Basic/Targets/Sparc.cpp

244–247

Yes, I have verified that GCC behaves the exact same way as this change and I don't see any reason not to mimic the exact same behavior in clang for maximum compatibility.

[glaubitz]

clang/lib/Basic/Targets/Sparc.cpp

244–247

FWIW, I meant GCC on the various BSDs. I do not think it's a wise idea to have clang deviate from what GCC does as only this way we can guarantee that everything that compiles with GCC will compile with clang.

[brad]

clang/lib/Basic/Targets/Sparc.cpp

244–247

Besides, have you checked what gcc on the BSDs really does? They often neglect to get their changes upstream and what's in the gcc repo doesn't necessarily represent what they actually use.

What is upstream is what we do. There are no local patches that change behavior in this particular area.

[glaubitz]

Gentle ping.

[joerg]

The NetBSD part looks ok, but also lacks proper testing. I'm not sure anyone but Linux cares at all otherwise as they lack 32bit SPARC support.

[glaubitz]

In D98574#2654686, @joerg wrote:

The NetBSD part looks ok, but also lacks proper testing. I'm not sure anyone but Linux cares at all otherwise as they lack 32bit SPARC support.

Well, there were no tests for NetBSD before, so I didn't change anything in this regard. I only changed clang to behave as gcc on SPARC and I
think that's the reasonable thing to do to avoid any compatibility issues when building with clang instead of gcc on any SPARC target.

[glaubitz]

Ping.

[hvdijk]

clang/lib/Basic/Targets/Sparc.cpp

244–247

(Copying here what I had already replied privately a while back) It worries me that this switch statement only handles known operating systems (Linux, FreeBSD, NetBSD, OpenBSD, Solaris) when we also have code to allow SparcV9TargetInfo to be created without an operating system in clang/lib/Basic/Targets.cpp. Either there should be a default case that is properly handled, or if that actually cannot happen, there should be an assert that it doesn't happen.

I agree with the earlier comments that there should be nothing wrong with defining more macros than GCC, if the macros make sense. For the SparcV9TargetInfo class, my impression is that the macros make sense. For the SparcV8TargetInfo class with a V9 CPU, reading the discussion in D86621, if Oracle say that __sparcv9 is only for 64-bit mode, GCC also only defines it for 64-bit mode, glibc assumes that __sparcv9 implies 64-bit mode, etc. then SparcV8TargetInfo should not be defining __sparcv9. Your changes to SparcV8TargetInfo should be enough to fix bug 49562, right? If so, would it be okay to update this diff with just that?

[glaubitz]

clang/lib/Basic/Targets/Sparc.cpp

244–247

Either there should be a default case that is properly handled, or if that actually cannot happen, there should be an assert that it doesn't happen.

OK, I can enable all definitions by default.

I agree with the earlier comments that there should be nothing wrong with defining more macros than GCC, if the macros make sense.

My argument is that I don't want to break existing software. We are here because I ran into an FTBFS because GCC and Clang were deviating from what they are defining.

I fully agree that what GCC does it not necessarily logically correct. But if we mimic GCC are making sure that we don't break any existing software.

[glaubitz]

Made changes according to review and dropped every change except removing sparcv9 and sparcv9__.

[glaubitz]

Updated the patch as requested and dropped all changes except the removal of sparcv9 and sparcv9__.

[glaubitz]

Minor improvement to the commit description.

[jrtc27]

It's been a while so I've since forgotten the details, but this looks fine now to me

[glaubitz]

Could someone land this for me? Thanks!

[MaskRay]

Testing and will push it.

clang/lib/Basic/Targets/Sparc.cpp

if (getTriple().getOS() == llvm::Triple::Solaris)
     Builder.defineMacro("__sparcv8");
   else {
     switch (getCPUGeneration(CPU)) {
     case CG_V8:
       Builder.defineMacro("__sparcv8");
       Builder.defineMacro("__sparcv8__");
       break;
     case CG_V9:
-      Builder.defineMacro("__sparcv9");
-      Builder.defineMacro("__sparcv9__");
       Builder.defineMacro("__sparc_v9__");
       break;
     }
   }
   if (getTriple().getVendor() == llvm::Triple::Myriad) {
     std::string MyriadArchValue, Myriad2Value;
     Builder.defineMacro("__sparc_v8__");
     Builder.defineMacro("__leon__");
     switch (CPU) {
     case CK_MYRIAD2100:
       MyriadArchValue = "__ma2100";
       Myriad2Value = "1";
       break;
     case CK_MYRIAD2150:

 
 void SparcV9TargetInfo::getTargetDefines(const LangOptions &Opts,
                                          MacroBuilder &Builder) const {
   SparcTargetInfo::getTargetDefines(Opts, Builder);
   Builder.defineMacro("__sparcv9");
   Builder.defineMacro("__arch64__");
   // Solaris doesn't need these variants, but the BSDs do.
   if (getTriple().getOS() != llvm::Triple::Solaris) {
     Builder.defineMacro("__sparc64__");
     Builder.defineMacro("__sparc_v9__");
     Builder.defineMacro("__sparcv9__");
   }
 
   Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1");
   Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_2");
   Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4");
   Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8");
 }
 
 void SparcV9TargetInfo::fillValidCPUList(
     SmallVectorImpl<StringRef> &Values) const {
   for (const SparcCPUInfo &Info : CPUInfo)
     if (Info.Generation == CG_V9)
       Values.push_back(Info.Name);
 }

clang/test/Preprocessor/predefined-arch-macros.c

 // CHECK_SPARC-NOT: #define __sparcv9__ 1
 
 // RUN: %clang -mcpu=v9 -E -dM %s -o - 2>&1 \
 // RUN:     -target sparc-unknown-linux \
 // RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_SPARC-V9
 // CHECK_SPARC-V9-NOT: #define __sparcv8 1
 // CHECK_SPARC-V9-NOT: #define __sparcv8__ 1
 // CHECK_SPARC-V9: #define __sparc_v9__ 1
-// CHECK_SPARC-V9: #define __sparcv9 1
-// CHECK_SPARC-V9: #define __sparcv9__ 1
+// CHECK_SPARC-V9-NOT: #define __sparcv9 1
+// CHECK_SPARC-V9-NOT: #define __sparcv9__ 1
 
 // RUN: %clang -E -dM %s -o - 2>&1 \
 // RUN:     -target sparc-sun-solaris \
 // RUN:   | FileCheck -match-full-lines %s -check-prefix=CHECK_SPARC_SOLARIS_GCC_ATOMICS
 // CHECK_SPARC_SOLARIS_GCC_ATOMICS: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_1 1
 // CHECK_SPARC_SOLARIS_GCC_ATOMICS: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_2 1
 // CHECK_SPARC_SOLARIS_GCC_ATOMICS: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_4 1
 // CHECK_SPARC_SOLARIS_GCC_ATOMICS: #define __GCC_HAVE_SYNC_COMPARE_AND_SWAP_8 1