linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: dave.martin@linaro.org (Dave Martin)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH-WIP 01/13] xen/arm: use r12 to pass the hypercall number to the hypervisor
Date: Thu, 8 Mar 2012 12:17:55 +0000	[thread overview]
Message-ID: <20120308121755.GA2703@linaro.org> (raw)
In-Reply-To: <4F5882BF.1030104@arm.com>

On Thu, Mar 08, 2012 at 09:58:23AM +0000, Richard Earnshaw wrote:
> On 02/03/12 21:15, Nicolas Pitre wrote:
> > [ coming back from vacation and trying to catch up ]
> >
> > On Wed, 29 Feb 2012, Dave Martin wrote:
> >
> >> Just had a chat with some tools guys -- apparently, when passing register
> >> arguments to gcc inline asms there really isn't a guarantee that those
> >> variables will be in the expected registers on entry to the inline asm.
> >>
> >> If gcc reorders other function calls or other code around the inline asm
> >> (which it can do, except under certain controlled situations), then
> >> intervening code can clobber any registers in general.
> >
> > I'm hearing this argument about once every year or so for the last 8
> > years.  I think that the tools people are getting confused between
> > themselves as you may get a different interpretation of what gcc should
> > do depending to whom you happen to talk to.
> >
> > I did submit a bug to gcc in 2004 about this:
> >
> >       http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15089
> >
> > You can see the confusion among gcc developers lurking there.
> >
> > So let's quote the relevant gcc documentation:
> >
> > -> * C Extensions::    GNU extensions to the C language family.
> >    -> * Explicit Reg Vars::   Defining variables residing in specified registers.
> >
> > |GNU C allows you to put a few global variables into specified hardware
> > |registers.  You can also specify the register in which an ordinary
> > |register variable should be allocated.
> > |
> > |   * Global register variables reserve registers throughout the program.
> > |     This may be useful in programs such as programming language
> > |     interpreters which have a couple of global variables that are
> > |     accessed very often.
> > |
> > |   * Local register variables in specific registers do not reserve the
> > |     registers, except at the point where they are used as input or
> > |     output operands in an `asm' statement and the `asm' statement
> > |     itself is not deleted.  The compiler's data flow analysis is
> > |     capable of determining where the specified registers contain live
> > |     values, and where they are available for other uses.  Stores into
> > |     local register variables may be deleted when they appear to be
> > |     dead according to dataflow analysis.  References to local register
> > |     variables may be deleted or moved or simplified.
> > |
> > |     These local variables are sometimes convenient for use with the
> > |     extended `asm' feature (*note Extended Asm::), if you want to
> > |     write one output of the assembler instruction directly into a
> > |     particular register.  (This will work provided the register you
> > |     specify fits the constraints specified for that operand in the
> > |     `asm'.)
> >
> >       -> * Local Reg Vars::
> >
> > [...]
> >
> > | Defining such a register variable does not reserve the register; it
> > |remains available for other uses in places where flow control
> > |determines the variable's value is not live.
> > |
> > | This option does not guarantee that GCC will generate code that has
> > |this variable in the register you specify at all times.  You may not
> > |code an explicit reference to this register in the _assembler
> > |instruction template_ part of an `asm' statement and assume it will
> > |always refer to this variable.  However, using the variable as an `asm'
> > |_operand_ guarantees that the specified register is used for the
> > |operand.

Hmmm, it's a while since I saw that documentation, and it had clearly
fallen out of my head when I made my previous statements...

> >
> > So, to me, the gcc documentation is perfectly clear on this topic.
> > there really _is_ a guarantee that those asm marked variables will be in
> > the expected registers on entry to the inline asm, given that the
> > variable is _also_ listed as an operand to the asm statement.  But only
> > in that case.
> >
> > It is true that gcc may reorder other function calls or other code
> > around the inline asm and then intervening code can clobber any
> > registers.  Then it is up to gcc to preserve the variable's content
> > elsewhere when its register is used for other purposes, and restore it
> > when some inline asm statement is referring to it.
> >
> > And if gcc does not do this then it is buggy.  Version 3.4.0 of gcc was
> > buggy.  No other gcc versions in the last 7 years had such a problem or
> > the __asmeq macro in the kernel would have told us.
> >
> >> Or, to summarise another way, there is no way to control which register
> >> is used to pass something to an inline asm in general (often we get away
> >> with this, and there are a lot of inline asms in the kernel that assume
> >> it works, but the more you inline the more likely you are to get nasty
> >> surprises).
> >
> > This statement is therefore unfounded and wrong.  Please direct the
> > tools guy who mislead you to the above gcc documentation.
> >
> 
> The problem is not really about re-ordering functions but about implicit
> functions that come from the source code; for example
> 
> int foo (int a, int b)
> {
>   register int x __asm__("r0") = 33;
> 
>   register int c __asm__("r1") = a / b; /* Ooops, clobbers r0 with
> division function call.  */
> 
>   asm ("svc 0" : : "r" (x));
> }

|   * Local register variables in specific registers do not reserve the
|     registers, except at the point where they are used as input or
|     output operands in an `asm' statement and the `asm' statement
|     itself is not deleted.  The compiler's data flow analysis is

So, I guess the issue is how to interpret this statement in the context
of the above code:  i.e., what does it mean for a register to be reserved
for a local register variable?

"The above paragraph says that Local register variables [do] reserve the
registers _[at] the point_ where they are used as input or output
operands in an `asm' statement and the `asm' statement itself is not
deleted." (my emphasis)

Under that reading, r0 must be reserved for x on entry to the asm, but
not necessarily at points preceding that.  If the asm sees anything in
r0 except for x, that would be noncompliant with the above paragraph.

Nevertheless, a slightly modified version of the above which does not
allow gcc to optimise the asm away does trigger just the kind of
behaviour you describe:

int foo(int a, int b)
{
	register int x asm("r0") = 33;
	register int c asm("r1") = a / b;

	asm("svc 0" : "+r" (x) : "r" (c));

	return x;
}

 -->

00000000 <foo>:
   0:   e92d4008        push    {r3, lr}
   4:   ebfffffe        bl      0 <__aeabi_idiv>
   8:   e1a01000        mov     r1, r0
   c:   ef000000        svc     0x00000000
  10:   e8bd8008        pop     {r3, pc}


This is doubly weird: x is an I/O to the asm with a "+r" constraint,
so even if the asm("rX") assignments are not guaranteed, then x should
be _somewhere_ on entry to the asm (even if not in r0).  But it is
completely gone.

Is this allowed, or wrong?  I don't see how this can be rationalised
with the gcc documentation that Nico quoted.

Have I missed something?

Cheers
---Dave

  reply	other threads:[~2012-03-08 12:17 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-02-23 17:47 [PATCH-WIP 00/13] xen/arm: receive Xen events and initialize xenbus Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 01/13] xen/arm: use r12 to pass the hypercall number to the hypervisor Stefano Stabellini
2012-02-27 16:27   ` Ian Campbell
2012-02-27 18:03     ` Dave Martin
2012-02-27 19:33       ` Ian Campbell
2012-02-28 10:20         ` Dave Martin
2012-02-28 10:48           ` Ian Campbell
2012-02-28 12:28             ` Stefano Stabellini
2012-02-29  9:34               ` Dave Martin
2012-02-29  9:56                 ` Ian Campbell
2012-02-29 11:47                   ` Dave Martin
2012-02-29 12:58                   ` Dave Martin
2012-02-29 14:44                     ` Ian Campbell
2012-03-01  9:35                       ` Dave Martin
2012-03-01 10:12                       ` Russell King - ARM Linux
2012-03-02 21:19                       ` Nicolas Pitre
2012-02-29 14:52                     ` Stefano Stabellini
2012-03-01  9:51                       ` Dave Martin
2012-03-01 10:10                     ` Russell King - ARM Linux
2012-03-01 10:27                       ` Dave Martin
2012-03-01 10:35                         ` Russell King - ARM Linux
2012-03-01 12:12                           ` Stefano Stabellini
2012-03-02 21:15                     ` Nicolas Pitre
2012-03-08  9:58                       ` Richard Earnshaw
2012-03-08 12:17                         ` Dave Martin [this message]
2012-03-08 17:21                         ` Nicolas Pitre
2012-03-08 18:47                           ` Richard Earnshaw
2012-03-09 15:58                             ` Dave Martin
2012-03-09 16:20                               ` Nicolas Pitre
2012-03-09 17:38                                 ` Richard Earnshaw
2012-02-27 21:05     ` Peter Maydell
2012-02-28 10:12       ` Ian Campbell
2012-02-27 17:53   ` Dave Martin
2012-02-27 19:48     ` Ian Campbell
2012-02-28  9:46       ` Dave Martin
2012-02-28 10:07         ` Ian Campbell
2012-02-28 12:21         ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 02/13] xen/arm: introduce privcmp, physdev_op and memory_op hypercalls Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 03/13] xen/arm: mmu.h and page.h related definitions Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 04/13] xen/arm: sync_bitops Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 05/13] xen/arm: empty implementation of grant_table arch specific functions Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 06/13] xen/arm: missing includes Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 07/13] xen/arm: receive xen events on arm Stefano Stabellini
2012-02-24 11:12   ` David Vrabel
2012-02-24 12:23     ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 08/13] xen/arm: fix arm xen guest handle definitions Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 09/13] xen/arm: shared_info and start_info Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 10/13] xen/arm: empty implementation of xen_remap_domain_mfn_range Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 11/13] xen/arm: Introduce xen_pfn_t for pfn and mfn types Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 12/13] xen/arm: compile and run xenbus Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 13/13] xen/arm: compile grant-table features events and xenbus, do not compile pci Stefano Stabellini

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=20120308121755.GA2703@linaro.org \
    --to=dave.martin@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    /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
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).