Re: [RFC] Function Multi Versioning on Arm
On 7/18/22 12:36, Daniel Kiss via Gcc wrote: > Hello, > > We are going to add Function Multiversioning [1] support to Arm architectures. > The specification is made public as beta[2] to ensure toolchain that follows > Arm > C Language Extension will implement it in the same way. > > A few tweaks considered to make the developers' life easier. > Since the `target` attribute is used widely on Arm, we would like to > introduce a > new attribute `target_version` to avoid confusion and possible deployment > problems. The `target_clones` attribute will be supported too. Also the > “default” > version to be made optional. > > We are looking for feedback on the specification (reply, github works too). > > Thanks so much, > Daniel > > [1] https://gcc.gnu.org/onlinedocs/gcc/Function-Multiversioning.html > [2] > https://github.com/ARM-software/acle/blob/main/main/acle.md#function-multi-versioning > Hey. I've just spent some time looking into https://gitlab.com/x86-psABIs/i386-ABI ABI and I noticed you may define something similar to: GNU 0x0020 NT_GNU_PROPERTY_TYPE_0 Properties: x86 ISA needed: x86-64-baseline, x86-64-v2, x86-64-v3, x86-64-v4 x86 feature used: x86 Where GNU_PROPERTY_X86_FEATURE_2_NEEDED has only 4B pr_data field. In you case you have ~60 features or so, which barely fits into 8B even. If you plan to add a something similar (GNU_PROPERTY_AARCH64_FEATURE_NEEDED or something similar), keep that in mind. Cheers, Martin
-Warray-bounds interprets int *var as int var[0] ?
The following code throws a warning which I do not understand.
Purpose is to save and restore SREG, which is a special function
register (SFR) defined by its hardware address as:
#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__))
which is the common C idiom to define such an SFR. The C code is:
typedef __UINT8_TYPE__ uint8_t;
#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__))
static __inline__ uint8_t __iCliRetVal (void)
{
__asm__ __volatile__ ("cli" ::: "memory");
return 1;
}
static __inline__ void __iRestore (const uint8_t *__s)
{
SREG = *__s;
__asm__ volatile ("" ::: "memory");
}
void foo (void)
{
for (uint8_t sreg_save __attribute__((__cleanup__(__iRestore))) = SREG,
__ToDo = __iCliRetVal();
__ToDo ;
__ToDo = 0 )
{
__asm ("nop");
}
}
The documentation of attribute cleanup says that the function provided
to cleanup (__iRestore) must take a pointer type that is compatible with
the attributed variable, which is the case. The warning is:
avr-gcc-13 -c foo-i.c -mmcu=atmega8 -Os -Wall -save-temps -dumpbase ""
foo-i.c: In function 'foo':
foo-i.c:20:71: warning: array subscript 0 is outside array bounds of
'volatile uint8_t[0]' {aka 'volatile unsigned char[]'} [-Warray-bounds]
20 |for (uint8_t sreg_save
__attribute__((__cleanup__(__iRestore))) = SREG,
|
~^~~~
In function '__iRestore',
inlined from 'foo' at foo-i.c:20:17:
foo-i.c:13:42: warning: array subscript 0 is outside array bounds of
'volatile uint8_t[0]' {aka 'volatile unsigned char[]'} [-Warray-bounds]
13 | SREG = *__s;
| ^
To me this looks like a GCC problem, and older versions of the compiler
don't complain. Or is there actually an issue with that code? Purpose
of the code is to save / restore SREG around a block of code, "nop" in
the example.
The warning complains about the places that are using SREG, so it that
macro definition wrong?
Thanks in advance,
Johann
> avr-gcc-13 -v
Using built-in specs.
Reading specs from
/home/DATA/gnu/install/gcc-master-avr/bin/../lib/gcc/avr/13.0.0/device-specs/specs-avr2
COLLECT_GCC=avr-gcc-13
COLLECT_LTO_WRAPPER=/home/DATA/gnu/install/gcc-master-avr/bin/../libexec/gcc/avr/13.0.0/lto-wrapper
Target: avr
Configured with: ../../source/gcc-master/configure --target=avr
--disable-nls --with-dwarf2 --enable-languages=c,c++ --with-gnu-as
--with-gnu-ld --disable-shared --with-fixed-point=no
--prefix=/home/john/gnu/install/gcc-master-avr --enable-checking=release
Thread model: single
Supported LTO compression algorithms: zlib
gcc version 13.0.0 20221103 (experimental) (GCC)
Re: -Warray-bounds interprets int *var as int var[0] ?
On Wed, Nov 23, 2022 at 9:15 AM Georg-Johann Lay wrote:
>
> The following code throws a warning which I do not understand.
>
> Purpose is to save and restore SREG, which is a special function
> register (SFR) defined by its hardware address as:
>
> #define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__))
>
> which is the common C idiom to define such an SFR. The C code is:
>
>
> typedef __UINT8_TYPE__ uint8_t;
>
> #define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__))
>
> static __inline__ uint8_t __iCliRetVal (void)
> {
> __asm__ __volatile__ ("cli" ::: "memory");
> return 1;
> }
>
> static __inline__ void __iRestore (const uint8_t *__s)
> {
> SREG = *__s;
> __asm__ volatile ("" ::: "memory");
> }
>
> void foo (void)
> {
>
> for (uint8_t sreg_save __attribute__((__cleanup__(__iRestore))) = SREG,
> __ToDo = __iCliRetVal();
> __ToDo ;
> __ToDo = 0 )
> {
> __asm ("nop");
> }
> }
>
>
> The documentation of attribute cleanup says that the function provided
> to cleanup (__iRestore) must take a pointer type that is compatible with
> the attributed variable, which is the case. The warning is:
>
> avr-gcc-13 -c foo-i.c -mmcu=atmega8 -Os -Wall -save-temps -dumpbase ""
> foo-i.c: In function 'foo':
> foo-i.c:20:71: warning: array subscript 0 is outside array bounds of
> 'volatile uint8_t[0]' {aka 'volatile unsigned char[]'} [-Warray-bounds]
> 20 |for (uint8_t sreg_save
> __attribute__((__cleanup__(__iRestore))) = SREG,
>|
> ~^~~~
> In function '__iRestore',
> inlined from 'foo' at foo-i.c:20:17:
> foo-i.c:13:42: warning: array subscript 0 is outside array bounds of
> 'volatile uint8_t[0]' {aka 'volatile unsigned char[]'} [-Warray-bounds]
> 13 | SREG = *__s;
>| ^
>
> To me this looks like a GCC problem, and older versions of the compiler
> don't complain. Or is there actually an issue with that code? Purpose
> of the code is to save / restore SREG around a block of code, "nop" in
> the example.
>
> The warning complains about the places that are using SREG, so it that
> macro definition wrong?
Either you need to use --param=min-pagesize=0 as an option or you need
to modify the avr backend to set that by default.
Thanks,
Andrew Pinski
>
> Thanks in advance,
>
> Johann
>
>
> > avr-gcc-13 -v
> Using built-in specs.
> Reading specs from
> /home/DATA/gnu/install/gcc-master-avr/bin/../lib/gcc/avr/13.0.0/device-specs/specs-avr2
> COLLECT_GCC=avr-gcc-13
> COLLECT_LTO_WRAPPER=/home/DATA/gnu/install/gcc-master-avr/bin/../libexec/gcc/avr/13.0.0/lto-wrapper
> Target: avr
> Configured with: ../../source/gcc-master/configure --target=avr
> --disable-nls --with-dwarf2 --enable-languages=c,c++ --with-gnu-as
> --with-gnu-ld --disable-shared --with-fixed-point=no
> --prefix=/home/john/gnu/install/gcc-master-avr --enable-checking=release
> Thread model: single
> Supported LTO compression algorithms: zlib
> gcc version 13.0.0 20221103 (experimental) (GCC)
>
Re: -Warray-bounds interprets int *var as int var[0] ?
Am 23.11.22 um 18:18 schrieb Andrew Pinski:
On Wed, Nov 23, 2022 at 9:15 AM Georg-Johann Lay wrote:
The following code throws a warning which I do not understand.
Purpose is to save and restore SREG, which is a special function
register (SFR) defined by its hardware address as:
#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__))
which is the common C idiom to define such an SFR. The C code is:
typedef __UINT8_TYPE__ uint8_t;
#define SREG (*(volatile uint8_t*) (0x3F + __AVR_SFR_OFFSET__))
static __inline__ uint8_t __iCliRetVal (void)
{
__asm__ __volatile__ ("cli" ::: "memory");
return 1;
}
static __inline__ void __iRestore (const uint8_t *__s)
{
SREG = *__s;
__asm__ volatile ("" ::: "memory");
}
void foo (void)
{
for (uint8_t sreg_save __attribute__((__cleanup__(__iRestore))) = SREG,
__ToDo = __iCliRetVal();
__ToDo ;
__ToDo = 0 )
{
__asm ("nop");
}
}
The documentation of attribute cleanup says that the function provided
to cleanup (__iRestore) must take a pointer type that is compatible with
the attributed variable, which is the case. The warning is:
avr-gcc-13 -c foo-i.c -mmcu=atmega8 -Os -Wall -save-temps -dumpbase ""
foo-i.c: In function 'foo':
foo-i.c:20:71: warning: array subscript 0 is outside array bounds of
'volatile uint8_t[0]' {aka 'volatile unsigned char[]'} [-Warray-bounds]
20 |for (uint8_t sreg_save
__attribute__((__cleanup__(__iRestore))) = SREG,
|
~^~~~
In function '__iRestore',
inlined from 'foo' at foo-i.c:20:17:
foo-i.c:13:42: warning: array subscript 0 is outside array bounds of
'volatile uint8_t[0]' {aka 'volatile unsigned char[]'} [-Warray-bounds]
13 | SREG = *__s;
| ^
To me this looks like a GCC problem, and older versions of the compiler
don't complain. Or is there actually an issue with that code? Purpose
of the code is to save / restore SREG around a block of code, "nop" in
the example.
The warning complains about the places that are using SREG, so it that
macro definition wrong?
Either you need to use --param=min-pagesize=0 as an option or you need
to modify the avr backend to set that by default.
Thanks,
Andrew Pinski
Ok, thanks. I filed it as PR107842.
Johann
> avr-gcc-13 -v
Using built-in specs.
Reading specs from
/home/DATA/gnu/install/gcc-master-avr/bin/../lib/gcc/avr/13.0.0/device-specs/specs-avr2
COLLECT_GCC=avr-gcc-13
COLLECT_LTO_WRAPPER=/home/DATA/gnu/install/gcc-master-avr/bin/../libexec/gcc/avr/13.0.0/lto-wrapper
Target: avr
Configured with: ../../source/gcc-master/configure --target=avr
--disable-nls --with-dwarf2 --enable-languages=c,c++ --with-gnu-as
--with-gnu-ld --disable-shared --with-fixed-point=no
--prefix=/home/john/gnu/install/gcc-master-avr --enable-checking=release
Thread model: single
Supported LTO compression algorithms: zlib
gcc version 13.0.0 20221103 (experimental) (GCC)
Using size_t to crash on off-by-one errors (was: size_t vs long.)
Hi,
On 11/18/22 00:04, Alejandro Colomar wrote:
The main advantage of this code compared to the equivalent ssize_t or
ptrdiff_t or idx_t code is that if you somehow write an off-by-one error, and
manage to access the array at [-1], if i is unsigned you'll access
[SIZE_MAX], which will definitely crash your program.
That's not true on the vast majority of today's platforms, which don't have
subscript checking, and for which a[-1] is treated the same way a[SIZE_MAX]
is. On my platform (Fedora 36 x86-64) the same machine code is generated for
'a' and 'b' for the following C code.
#include
int a(int *p) { return p[-1]; }
int b(int *p) { return p[SIZE_MAX]; }
Hmm, this seems to be true in my platform (amd64) per the experiment I just did:
$ cat s.c
#include
char
f(char *p, ssize_t i)
{
return p[i];
}
$ cat u.c
#include
char
f(char *p, size_t i)
{
return p[i];
}
$ cc -Wall -Wextra -Werror -S -O3 s.c u.c
$ diff -u u.s s.s
--- u.s 2022-11-17 23:41:47.773805041 +0100
+++ s.s 2022-11-17 23:41:47.761805265 +0100
@@ -1,15 +1,15 @@
- .file "u.c"
+ .file "s.c"
.text
.p2align 4
.globl f
.type f, @function
f:
-.LFB0:
+.LFB6:
.cfi_startproc
movzbl (%rdi,%rsi), %eax
ret
.cfi_endproc
-.LFE0:
+.LFE6:
.size f, .-f
.ident "GCC: (Debian 12.2.0-9) 12.2.0"
.section .note.GNU-stack,"",@progbits
It seems a violation of the standard, isn't it?
The operator [] doesn't have a type, and an argument to it should be treated
with whatever type it has after default promotions. If I pass a size_t to it,
the type should be unsigned, and that should be preserved, by accessing the
array at a high value, which the compiler has no way to know if it will exist or
not, by that function definition. The extreme of -1 and SIZE_MAX might be not
the best one, since we would need a pointer to be 0 to be accessible at
[SIZE_MAX], but if you replace those by -RANDOM, and (size_t)-RANDOM, then the
compiler definitely needs to generate different code, yet it doesn't.
I'm guessing this is an optimization by GCC knowing that we will never be close
to using the whole 64-bit address space. If we use int and unsigned, things
change:
$ cat s.c
char
f(char *p, int i)
{
return p[i];
}
alx@asus5775:~/tmp$ cat u.c
char
f(char *p, unsigned i)
{
return p[i];
}
$ cc -Wall -Wextra -Werror -S -O3 s.c u.c
$ diff -u u.s s.s
--- u.s 2022-11-17 23:44:54.446318186 +0100
+++ s.s 2022-11-17 23:44:54.434318409 +0100
@@ -1,4 +1,4 @@
- .file "u.c"
+ .file "s.c"
.text
.p2align 4
.globl f
@@ -6,7 +6,7 @@
f:
.LFB0:
.cfi_startproc
- movl %esi, %esi
+ movslq %esi, %rsi
movzbl (%rdi,%rsi), %eax
ret
.cfi_endproc
I'm guessing that GCC doesn't do the assumption here, and I guess the unsigned
version would crash, while the signed version would cause nasal demons. Anyway,
now that I'm here, I'll test it:
$ cat s.c
[[gnu::noipa]]
char
f(char *p, int i)
{
return p[i];
}
int main(void)
{
int i = -1;
char c[4];
return f(c, i);
}
$ cc -Wall -Wextra -Werror -O3 s.c
$ ./a.out
$ echo $?
0
$ cat u.c
[[gnu::noipa]]
char
f(char *p, unsigned i)
{
return p[i];
}
int main(void)
{
unsigned i = -1;
char c[4];
return f(c, i);
}
$ cc -Wall -Wextra -Werror -O3 u.c
$ ./a.out
Segmentation fault
I get this SEGV difference consistently. I CCed gcc@ in case they consider this
to be something they want to address. Maybe the optimization is important for
size_t-sized indices, but if it is not, I'd prefer getting the SEGV for SIZE_MAX.
After some though, of course the compiler can't produce any different code,
since pointers are 64 bits. A different story would be if pointers were 128
bits, but that might cause its own issues; should sizes be still 64 bits? or 128
bits? Maybe using a configurable size_t would be interesting for debugging.
Anyway, it's good to know that tweaking size_t to be 32 bits in some debug
builds might help catch some off-by-one errors.
Cheers,
Alex
--
-fanalyzer: Questions on C vs CPP + use of GCC attr's like malloc()/access()
Hey all, just a few questions about the fantastic GCC Static Analyzer: - It's stated that support for C++ vs C is very limited. Does this apply if you're writing C++ that is very similar-looking to C and uses few of C++'s advanced features? - I noticed that in C++, the "gnu::malloc" attributes don't seem to report "warning: leak of 'xxx_alloc()' [CWE-401] [-Wanalyzer-malloc-leak]", is this normal? - Is it worthwhile to spend time annotating your method signatures with attributes like "malloc" and "access"? Do these aid the -fanalyzer passes? For instance: [[gnu::malloc]] [[gnu::malloc(HeapPage_free, 1)]] [[gnu::returns_nonnull]] struct HeapPage* HeapPage_alloc(); [[gnu::access(read_write, 1, 3)]] struct RecordID HeapPage_insert_record(struct HeapPage* page, const char* record, uint32_t record_length); That's quite a significant bit of annotation-noise tacked on to the function, so I wanted to be sure it's worth the investment! Thank you =) Gavin Ray
