glaubitz 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__");
----------------
hvdijk wrote:
> 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?
> 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.
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
