Linux-RISC-V Archive on lore.kernel.org
 help / color / Atom feed
From: Tobias Klauser <tklauser@distanz.ch>
To: Palmer Dabbelt <palmer@dabbelt.com>
Cc: linux-riscv@lists.infradead.org, aou@eecs.berkeley.edu,
	Paul Walmsley <paul.walmsley@sifive.com>
Subject: Re: [PATCH v2] riscv: don't specify -mno-save-restore when building with clang < 11
Date: Sat, 1 Aug 2020 12:30:17 +0200
Message-ID: <20200801103016.l2hs2g2vdsphkjbt@distanz.ch> (raw)
In-Reply-To: <mhng-e999ad7d-e45e-4e12-ac87-1b466705f41f@palmerdabbelt-glaptop1>

On 2020-07-31 at 18:16:28 +0200, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> On Fri, 31 Jul 2020 01:16:51 PDT (-0700), tklauser@distanz.ch wrote:
> > On 2020-07-31 at 05:58:13 +0200, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > > On Tue, 28 Jul 2020 21:44:28 PDT (-0700), tklauser@distanz.ch wrote:
> > > > Clang before version 11 doesn't support -msave-restore and
> > > > -mno-save-restore [1].
> > > >
> > > > [1] https://github.com/ClangBuiltLinux/linux/issues/804
> > > >
> > > > This avoids the following message when building with clang 10 and older:
> > > >
> > > >   '-save-restore' is not a recognized feature for this target (ignoring feature)
> > > >
> > > > Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> > > > ---
> > > > v2: use cc-option and check for clang version
> > > >
> > > >  arch/riscv/Makefile | 9 ++++++++-
> > > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > > > index fb6e37db836d..30e34946af86 100644
> > > > --- a/arch/riscv/Makefile
> > > > +++ b/arch/riscv/Makefile
> > > > @@ -44,7 +44,14 @@ riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
> > > >  KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
> > > >  KBUILD_AFLAGS += -march=$(riscv-march-y)
> > > >
> > > > -KBUILD_CFLAGS += -mno-save-restore
> > > > +KBUILD_CFLAGS += $(call cc-option,-mno-save-restore)
> > > > +# Clang versions less than 11 do not support save-restore. See
> > > > +# https://github.com/ClangBuiltLinux/linux/issues/804
> > > > +ifeq ($(CONFIG_CC_IS_CLANG), y)
> > > > +  ifeq ($(shell test $(CONFIG_CLANG_VERSION) -lt 110000; echo $$?),0)
> > > > +    KBUILD_CFLAGS := $(filter-out -mno-save-restore,$(KBUILD_CFLAGS))
> > > > +  endif
> > > > +endif
> > > >  KBUILD_CFLAGS += -DCONFIG_PAGE_OFFSET=$(CONFIG_PAGE_OFFSET)
> > > >
> > > >  ifeq ($(CONFIG_CMODEL_MEDLOW),y)
> > > 
> > > Ya, that seems worse ;).
> > > 
> > > Does this work?  I still get a -mno-save-restore on GCC, but I don't have an
> > > LLVM lying around to test it with.
> > 
> > Yeah, it should still emit -mno-save-restore on GCC and on Clang >= 11,
> > which is what it does in my tests here.
> > 
> > It is certainly rather brittle by relying on the specific Clang version
> > and not very nice to look at, so your solution would certainly be much
> > nicer. Unfortunately, with cc-option-skipwarn I still get:
> > 
> >   '-save-restore' is not a recognized feature for this target (ignoring feature)
> > 
> > when building with Clang 10 :( I think the issue is that Clang
> > "somewhat" accepts/handles the option  i.e. doesn't treat it as an
> > unknown option like others and thus doesn't exit with non-zero, like it
> > does for other options only available for gcc.
> 
> Sorry, I was asking if my patches below work.  Your stuff seems like it'd
> pretty obviously work, I just don't like sticking the version in there.  The
> patches are below.

Sorry, I should have been more explicit in my message. I was testing
with cc-option-skipwarn with your two patches applied already. But
unfortunately, the warning still appeared when building with Clang 10
:-(

The only way I can currently think of to detect this case using a
cc-option helper would be to somehow parse clang's stdout/stderr for the
message which IMO would be equally brittle as making the flag version
dependent :-(

> > 
> > > From 3e3ae8610db0abba9dd6a18e5d6b9bc8c495c2fc Mon Sep 17 00:00:00 2001
> > > From: Palmer Dabbelt <palmerdabbelt@google.com>
> > > Date: Thu, 30 Jul 2020 19:41:11 -0700
> > > Subject: [PATCH 1/2] Kbuild: add cc-option-skipwarn
> > > 
> > > The documentation describes the behavior of this function.  In theory we could
> > > just add -Werror to cc-option, but I don't want to chase around an infinate
> > > tail of weird build bugs due to compilers that have this odd behavior.
> > > 
> > > Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>
> > > ---
> > > Documentation/kbuild/makefiles.rst | 7 +++++++
> > > scripts/Kbuild.include             | 6 ++++++
> > > 2 files changed, 13 insertions(+)
> > > 
> > > diff --git a/Documentation/kbuild/makefiles.rst b/Documentation/kbuild/makefiles.rst
> > > index 6515ebc12b6f..dba9faf4af29 100644
> > > --- a/Documentation/kbuild/makefiles.rst
> > > +++ b/Documentation/kbuild/makefiles.rst
> > > @@ -507,6 +507,13 @@ more details, with real examples.
> > > 	cflags-y will be assigned no value if first option is not supported.
> > > 	Note: cc-option uses KBUILD_CFLAGS for $(CC) options
> > > 
> > > +   cc-option-skipwarn
> > > +	Some compilers emit a warning on unsupported options rather than an
> > > +	error.  This trips up cc-option, as the test compiler run will succeed
> > > +	only to spit out the warning for every file.  cc-option-skipwarn allows
> > > +	the build infastructure to detect compiles that behave this way, but
> > > +	otherwise acts exactly as cc-option does.
> > > +
> > >    cc-option-yn
> > > 	cc-option-yn is used to check if gcc supports a given option
> > > 	and return 'y' if supported, otherwise 'n'.
> > > diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include
> > > index 9a15fbf66aa1..2efd3a8890c7 100644
> > > --- a/scripts/Kbuild.include
> > > +++ b/scripts/Kbuild.include
> > > @@ -129,6 +129,12 @@ CC_OPTION_CFLAGS = $(filter-out $(GCC_PLUGINS_CFLAGS),$(KBUILD_CFLAGS))
> > > cc-option = $(call __cc-option, $(CC),\
> > > 	$(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS),$(1),$(2))
> > > 
> > > +# cc-option-skipwarn
> > > +# Usage: cflags-y += $(call cc-option-skipwarn,-mno-save-restore)
> > > +
> > > +cc-option-skipwarn = $(call __cc-option, $(CC),\
> > > +	$(KBUILD_CPPFLAGS) -Werror $(CC_OPTION_CFLAGS),$(1),$(2))
> > > +
> > > # cc-option-yn
> > > # Usage: flag := $(call cc-option-yn,-march=winchip-c6)
> > > cc-option-yn = $(call try-run,\
> > > --
> > > 2.28.0.163.g6104cc2f0b6-goog
> > > 
> > > 
> > > From 8509566a1a3480ea309fd81317d8cd6125b5e294 Mon Sep 17 00:00:00 2001
> > > From: Tobias Klauser <tklauser@distanz.ch>
> > > Date: Wed, 29 Jul 2020 06:44:28 +0200
> > > Subject: [PATCH 2/2] riscv: don't specify -mno-save-restore unless it's
> > > supported
> > > 
> > > Clang before version 11 doesn't support -msave-restore and
> > > -mno-save-restore [1].
> > > 
> > > [1] https://github.com/ClangBuiltLinux/linux/issues/804
> > > 
> > > This avoids the following message when building with clang 10 and older:
> > > 
> > >  '-save-restore' is not a recognized feature for this target (ignoring feature)
> > > 
> > > Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> > > [Palmer: Use cc-option-nowarn]
> > > Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>
> > > ---
> > > arch/riscv/Makefile | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
> > > index fb6e37db836d..532029c887f1 100644
> > > --- a/arch/riscv/Makefile
> > > +++ b/arch/riscv/Makefile
> > > @@ -44,7 +44,7 @@ riscv-march-$(CONFIG_RISCV_ISA_C)	:= $(riscv-march-y)c
> > > KBUILD_CFLAGS += -march=$(subst fd,,$(riscv-march-y))
> > > KBUILD_AFLAGS += -march=$(riscv-march-y)
> > > 
> > > -KBUILD_CFLAGS += -mno-save-restore
> > > +KBUILD_CFLAGS += $(call cc-option-skipwarn,-mno-save-restore)
> > > KBUILD_CFLAGS += -DCONFIG_PAGE_OFFSET=$(CONFIG_PAGE_OFFSET)
> > > 
> > > ifeq ($(CONFIG_CMODEL_MEDLOW),y)
> > > --
> > > 2.28.0.163.g6104cc2f0b6-goog
> > > 
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-28 13:12 [PATCH RESEND] riscv: don't specify -mno-save-restore when building with clang Tobias Klauser
2020-07-28 15:20 ` Palmer Dabbelt
2020-07-28 16:06   ` Tobias Klauser
2020-07-29  4:41     ` Tobias Klauser
2020-07-29  8:08       ` Jerome Forissier
2020-07-29  8:29         ` Tobias Klauser
2020-07-29  9:36           ` Jerome Forissier
2020-07-29  9:43             ` Jerome Forissier
2020-07-29  4:44 ` [PATCH v2] riscv: don't specify -mno-save-restore when building with clang < 11 Tobias Klauser
2020-07-31  3:58   ` Palmer Dabbelt
2020-07-31  8:16     ` Tobias Klauser
2020-07-31 16:16       ` Palmer Dabbelt
2020-08-01 10:30         ` Tobias Klauser [this message]
2020-08-03 20:28           ` Palmer Dabbelt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200801103016.l2hs2g2vdsphkjbt@distanz.ch \
    --to=tklauser@distanz.ch \
    --cc=aou@eecs.berkeley.edu \
    --cc=linux-riscv@lists.infradead.org \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RISC-V Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-riscv/0 linux-riscv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-riscv linux-riscv/ https://lore.kernel.org/linux-riscv \
		linux-riscv@lists.infradead.org
	public-inbox-index linux-riscv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-riscv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git