HI Jakub,
On 4/16/19 7:32 PM, Jakub Jelinek wrote:
On Tue, Apr 16, 2019 at 07:50:35PM +0200, Jakub Jelinek wrote:
> On Fri, Apr 12, 2019 at 05:10:48PM +0100, Ramana Radhakrishnan wrote:
> > No, that's not right. we should get rid of this.
>
> Here is a patch for that.
>
> Bootstrapped/regtested on armv7hl-linux-gnueabi, ok for trunk?
And here is the same thing for aarch64. Bootstrapped/regtested on
aarch64-linux, ok for trunk?
FWIW this looks ok to me implementation-wise (since I wrote that code a
few years ago).
I think it is better not to accept any spaces in there, than accepting it
only at the beginning and after , but not e.g. at the end of before ,
like the trunk currently does, furthermore, e.g. x86 or ppc don't allow
spaces there.
Thinking about it a bit more, I think it's a good idea to disallow
leading and trailing whitespaces.
But there could be a case for allowing whitespaces between separate
target attributes.
Personally, I would find it more readable to have a space after a comma.
Similarly, spaces are allowed in the general attribute syntax, for
example in our intrinsics header we have:
__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
That said, distinguishing between the two classes of whitespace is
probably more complexity than it's worth
and if other targets don't allow it then I won't let it block this patch.
Thanks,
Kyrill
2019-04-16 Jakub Jelinek <ja...@redhat.com>
PR target/89093
* config/aarch64/aarch64.c (aarch64_process_one_target_attr):
Don't skip
whitespace at the start of target attribute string.
* gcc.target/aarch64/pr89093.c: New test.
* gcc.target/aarch64/pr63304_1.c: Remove space from target string.
--- gcc/config/aarch64/aarch64.c.jj 2019-04-11 10:26:22.907293129
+0200
+++ gcc/config/aarch64/aarch64.c 2019-04-15 19:59:55.784226278
+0200
@@ -12536,10 +12536,6 @@ aarch64_process_one_target_attr (char *a
char *str_to_check = (char *) alloca (len + 1);
strcpy (str_to_check, arg_str);
- /* Skip leading whitespace. */
- while (*str_to_check == ' ' || *str_to_check == '\t')
- str_to_check++;
-
/* We have something like __attribute__ ((target ("+fp+nosimd"))).
It is easier to detect and handle it explicitly here rather than
going
through the machinery for the rest of the target attributes in this
--- gcc/testsuite/gcc.target/aarch64/pr89093.c.jj 2019-04-15
20:02:25.456788897 +0200
+++ gcc/testsuite/gcc.target/aarch64/pr89093.c 2019-04-15
20:02:04.433131260 +0200
@@ -0,0 +1,7 @@
+/* PR target/89093 */
+/* { dg-do compile } */
+
+__attribute__((target (" no-strict-align"))) void f1 (void) {} /* {
dg-error "is not valid" } */
+__attribute__((target (" general-regs-only"))) void f2 (void)
{} /* { dg-error "is not valid" } */
+#pragma GCC target (" general-regs-only") /* { dg-error "is not
valid" } */
+void f3 (void) {}
--- gcc/testsuite/gcc.target/aarch64/pr63304_1.c.jj 2017-09-13
16:22:19.795513580 +0200
+++ gcc/testsuite/gcc.target/aarch64/pr63304_1.c 2019-04-15
20:27:17.724847578 +0200
@@ -1,7 +1,7 @@
/* { dg-do assemble } */
/* { dg-options "-O1 --save-temps" } */
#pragma GCC push_options
-#pragma GCC target ("+nothing+simd, cmodel=small")
+#pragma GCC target ("+nothing+simd,cmodel=small")
int
cal (double a)
Jakub
