Page MenuHomePhabricator

[Sparc] Define the same macros for -mcpu=v9 as GCC on Linux and the BSDs
Needs ReviewPublic

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

Details

Summary

When targeting SPARC V8+ on Linux and the BSDs, GCC defines the macro
sparc_v9 only while clang also defines additional macros such as
sparcv9. When targeting SPARC V9, GCC defines sparc_v9__ on Linux
and the BSDs while other macros such as sparcv9 are defined on FreeBSD,
NetBSD and Solaris. On OpenBSD, GCC defines
sparcv9__ and not __sparcv9.

In order to avoid compatibility problems, make sure clang behaves as
GCC and defines the same macros depending on the target platform and
SPARC CPU baseline. 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__.

247–259

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

247–259

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
247–259

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
247–259

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
247–259

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
247–259

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.Sun, May 9, 11:24 AM
hvdijk added inline comments.
clang/lib/Basic/Targets/Sparc.cpp
247–259

(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?