From: Matthias Kaehlcke <email@example.com>
To: Mark Rutland <firstname.lastname@example.org>
Cc: "Ard Biesheuvel" <email@example.com>,
"Catalin Marinas" <firstname.lastname@example.org>,
"Will Deacon" <email@example.com>,
"Christoffer Dall" <firstname.lastname@example.org>,
"Marc Zyngier" <email@example.com>,
"Paolo Bonzini" <firstname.lastname@example.org>,
"Radim Krčmář" <email@example.com>, "Tejun Heo" <firstname.lastname@example.org>,
"Christoph Lameter" <email@example.com>,
"Vladimir Murzin" <firstname.lastname@example.org>,
"KVM devel mailing list" <email@example.com>,
"Grant Grundler" <firstname.lastname@example.org>,
"Greg Hackmann" <email@example.com>,
"Michael Davidson" <firstname.lastname@example.org>
Subject: Re: [PATCH v2] arm64: Add ASM modifier for xN register operands
Date: Fri, 28 Apr 2017 12:08:15 -0700 [thread overview]
Message-ID: <20170428190815.GT128305@google.com> (raw)
El Fri, Apr 28, 2017 at 03:33:33PM +0100 Mark Rutland ha dit:
> On Fri, Apr 28, 2017 at 08:18:52AM +0100, Ard Biesheuvel wrote:
> > On 27 April 2017 at 23:52, Matthias Kaehlcke <email@example.com> wrote:
> > > El Thu, Apr 27, 2017 at 12:02:56PM +0100 Mark Rutland ha dit:
> > >> On Wed, Apr 26, 2017 at 02:46:16PM -0700, Matthias Kaehlcke wrote:
> > >> > Many inline assembly statements don't include the 'x' modifier when
> > >> > using xN registers as operands. This is perfectly valid, however it
> > >> > causes clang to raise warnings like this:
> > >> >
> > >> > warning: value size does not match register size specified by the
> > >> > constraint and modifier [-Wasm-operand-widths]
> > >> > - asm volatile("strb %w0, [%1]" : : "rZ" (val), "r" (addr));
> > >> > + asm volatile("strb %w0, [%x1]" : : "rZ" (val), "r" (addr));
> > >>
> > >> In general, the '[%xN]' pattern looks *very* suspicious to me. Any
> > >> address must be 64-bit, so this would mask a legitimate warning.
> > >>
> > >> Given the prototype of this function the code if fine either way, but
> > >> were we to refactor things (e.g. making this a macro), that might not be
> > >> true.
> > >>
> > >> ... so I'm not sure it make sense to alter instances used for addresses.
> > >
> > > Good point, I'll leave instances dealing with addresses untouched for now.
> > >
> > OK, I am confused now. We started this thread under the assumption
> > that all unqualified placeholders are warned about by Clang. Given
> > that this appears not to be the case, could we please first find out
> > what causes the warnings? Is it necessary at all to add the x
> > modifiers for 64-bit types?
> FWIW, I grabbed a clang 4.0.0 binary and had a play.
> It looks like clang only warns when an operand is less than 64 bits
> wide, and there is no 'x' or 'w' modifier. Pointers a 64 bits wide, so
> never produce a warning.
> As far as I can tell, applying to both integers and pointers:
> * GCC and clang always treat %N as meaning xN for an r constraint, and
> you need to use %wN to get wN.
> * If an operand type is 64 bits in size, clang will not produce a warning
> regarding the operand size.
> * If an x or w modifier is used, clang will not produce a warning
> regarding the operand size, regardless of whether it matches the
> register size. Clang is happy for %wN to be used on a pointer type.
> * If an operand type is less than 64 bits in size, and neither an x or
> w modifier is used, clang will produce a warning as above.
> * If an operand type is greater than 64 bits in size, clang encounters
> an internal error.
> Given that, I think we *should not* use the x modifier to suppress this
> warning, as I think for those cases we have a potential bug as outlined
> in my prior reply.
> Instead, we should use a temporary 64-bit variable (or cast input
> operands to 64-bit), which avoids that and makes clang happy.
> I've included my test below. Note that clang will produce other errors for
> invalid asm (e.g. for mov w0, x0).
Thanks for your investigation!
I apologize for the noise, my expertise with inline assembly is
extremely limited, and admittedly I need a bit of handholding in this
area. Not without reason changes like this or the prefetch code are at
the very top of my clang stack (i.e. postponed until the other less
scary issues were solved). Hopefully the discussion was still useful.
I'll prepare a short patch that only fixes the warnings encountered in
my build in the way you suggested.
prev parent reply other threads:[~2017-04-28 19:08 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-26 21:46 [PATCH v2] arm64: Add ASM modifier for xN register operands Matthias Kaehlcke
2017-04-27 11:02 ` Mark Rutland
2017-04-27 22:52 ` Matthias Kaehlcke
2017-04-28 7:18 ` Ard Biesheuvel
2017-04-28 9:53 ` Mark Rutland
2017-04-28 10:20 ` Ard Biesheuvel
2017-04-28 10:32 ` Mark Rutland
2017-04-28 14:33 ` Mark Rutland
2017-04-28 14:43 ` Ard Biesheuvel
2017-04-28 14:47 ` Will Deacon
2017-04-28 19:08 ` Matthias Kaehlcke [this message]
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:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).