On 21/02/12 05:39, Barracuda wrote:
> Hello!
> I'm new to GCC internals, but I'm using GCC for couple of years.
> Yesterday I found that GCC does not support calling SWI routines from C/C++
> code.
> For example, in other ARM-targeted compiliers developer can use such syntax
> for function prototype:
> In ARM IAR:
> #pragma swi_number=0x15
> int some_call(int, int);
> In RVDS:
> __swi(0x15) int some_call(int, int);
> And then just call function as usual:
> a = some_call(5, 8);
> GCC lacks this feauture, so I've decided to go on Free Software way - if your
> need something, implement it yourself =)
> I can't write any testsuite, because I don't know how to do it, sorry. I
> tested some programs written for IAR, they compiled and launched successfully.
> I've also tested some programs with functions that don't use this attribute -
> they was unaffected by this patch. So I decided to send it to this mailing
> list.
> Changelog and patch included in attachment.
> I tested cross-compiling, host=i686-pc-linux-gnu and host=x86_64-pc-linux-gnu,
> target=arm-linux-gnueabi. Works stable and fine.
> 
> Barracuda
> 
> 
> Chlog.txt
> 
> 
> 2012-02-21  Labutin Ivan  <barracud...@bk.ru>
> 
>       * gcc/config/arm/arm.c: Added support for __attribute__ ((swi(x))).
>           With this, you can write function prototype:
>             int some_sys_call (int, char*) __attribute__ ((swi(0x15)));
>           Now, if you call it:
>             a = some_sys_call (5, "test");
>           it will produce following assembler output:
>             ...
>             MOV R1, #test_adr 
>             MOV R0, #5
>             SWI 0x15
>             ...
>           This is an analogue of 
>             #pragma swi_number=0x15 int some_sys_call (int, char *);
>           or
>             __swi(0x15) int some_sys_call (int, char*);
>           in other ARM-targeted compiliers.
>         * gcc/config/arm.md: Likewise.
>         * gcc/config/arm-protos.h: Likewise. 

Thanks for the patch, I think it should be a useful addition.  However,
before this can go in, there are a number of things that need addressing.

1) Do you have a copyright assignment in place with the FSF?  A
contribution of this size cannot go in without one.

2) We need some tests.  Changes of this nature without tests are likely
to get broken over time unless there's some way of checking that this
doesn't happen.

3) We need some documentation for users.  Things you need to cover
include: how to use the attribute, how this interacts with the procedure
call standard (incidentally, you just seem to take the default rules for
the PCS, is this what you intended?).  Furthermore, you need to explain
that this feature cannot be used if the SWI call number is not passed
either in the SWI instruction itself, or as a normal argument.  Linux is
a special example of this where the call number is passed in R7.

4) Have you tested this to ensure that it works with tail-calls?  I
can't see any evidence you've handled that, so

void f(int a)
{
  myswicall(a);
}

may well end up doing the wrong thing.

5) There are some coding standard issues to sort out.  Use
    }
  else
    {

Not

  } else {

6) No comments, not one... :-(

7) When you don't use an argument formal in the body of a function, you
need to mark it as unused, or you'll break native bootstraps.

8) Although you've added support for both ARM and Thumb, I don't see any
value range checking for the SWI call number.  The thumb instruction set
only supports SWI calls in the range 0-255, while ARM supports 0-0xffffff.

9) The patch suggests to me that it's been prepared against gcc-4.5.  We
need patch submissions against gcc-trunk.  Please can you rebase.

10) Finally, the name SWI is now deprecated in assembly syntax, I think
the attribute should use the new (actually it's not really that new now)
name, SVC.

For extra brownie points, you could even extend it to support the SMC
and HVC instructions.

R.

Reply via email to