This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by glaubitz on Mar 13 2021, 12:52 AM.

Details

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.

Diff Detail

Event Timeline

glaubitz created this revision.Mar 13 2021, 12:52 AM
glaubitz requested review of this revision.Mar 13 2021, 12:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2021, 12:52 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
brad added a comment.Mar 13 2021, 4:46 PM

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 requested changes to this revision.Mar 13 2021, 4:53 PM
jrtc27 added inline comments.
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.

This revision now requires changes to proceed.Mar 13 2021, 4:53 PM
glaubitz added inline comments.Mar 17 2021, 2:59 AM
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.

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 updated this revision to Diff 332312.Mar 22 2021, 9:06 AM
glaubitz retitled this revision from [Sparc] Define the same macros for -mcpu=v9 as GCC on Linux to [Sparc] Define the same macros for -mcpu=v9 as GCC on Linux and the BSDs.
glaubitz edited the summary of this revision. (Show Details)
glaubitz marked an inline comment as done.
glaubitz updated this revision to Diff 332320.Mar 22 2021, 9:16 AM
glaubitz edited the summary of this revision. (Show Details)
glaubitz added a comment.EditedMar 22 2021, 9:22 AM

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 added a comment.Mar 22 2021, 9:27 AM

-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 updated this revision to Diff 332339.Mar 22 2021, 9:51 AM
glaubitz edited the summary of this revision. (Show Details)

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

glaubitz updated this revision to Diff 332380.Mar 22 2021, 11:52 AM
glaubitz updated this revision to Diff 332567.Mar 23 2021, 1:58 AM
glaubitz edited the summary of this revision. (Show Details)

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

ro added inline comments.Mar 25 2021, 3:15 AM
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 marked an inline comment as done.Mar 25 2021, 3:20 AM
glaubitz added inline comments.
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 marked an inline comment as done.Mar 25 2021, 3:21 AM
glaubitz added inline comments.
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 added inline comments.Mar 26 2021, 1:18 PM
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.

joerg added a subscriber: joerg.Mar 28 2021, 12:59 PM

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.

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.

hvdijk added a subscriber: hvdijk.May 9 2021, 11:24 AM
hvdijk added inline comments.
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 added inline comments.Jun 7 2021, 2:43 AM
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 updated this revision to Diff 401950.Jan 21 2022, 5:01 AM

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

glaubitz updated this revision to Diff 401951.Jan 21 2022, 5:03 AM
glaubitz edited the summary of this revision. (Show Details)

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

glaubitz updated this revision to Diff 401953.Jan 21 2022, 5:05 AM
glaubitz edited the summary of this revision. (Show Details)

Minor improvement to the commit description.

glaubitz retitled this revision from [Sparc] Define the same macros for -mcpu=v9 as GCC on Linux and the BSDs to [Sparc] Don't define __sparcv9 and __sparcv9__ when targeting V8+.Jan 21 2022, 5:06 AM
hvdijk accepted this revision.Jan 21 2022, 5:08 AM
jrtc27 accepted this revision.Jan 21 2022, 8:29 AM

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

This revision is now accepted and ready to land.Jan 21 2022, 8:29 AM

Could someone land this for me? Thanks!

MaskRay accepted this revision.Jan 21 2022, 9:54 AM

Testing and will push it.

This revision was landed with ongoing or failed builds.Jan 21 2022, 9:57 AM
This revision was automatically updated to reflect the committed changes.