hvdijk added inline comments.
================
Comment at: clang/lib/Basic/Targets/Sparc.cpp:246-256
+ if (getTriple().getOS() == llvm::Triple::Linux) {
Builder.defineMacro("__sparc_v9__");
- Builder.defineMacro("__sparcv9__");
+ } else {
+ Builder.defineMacro("__sparcv9");
+ // Solaris doesn't need these variants, but the BSDs do.
+ if (getTriple().getOS() != llvm::Triple::Solaris) {
+ Builder.defineMacro("__sparc64__");
----------------
brad wrote:
> glaubitz wrote:
> > glaubitz wrote:
> > > ro wrote:
> > > > glaubitz wrote:
> > > > > jrtc27 wrote:
> > > > > > This doesn't need changing, we can define more things than GCC to
> > > > > > keep it simple.
> > > > > 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 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.
> > > 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.
> > 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.
> > 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.
(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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98574/new/
https://reviews.llvm.org/D98574
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
