On Fri, 5 Jul 2019 at 12:57, Jozef Lawrynowicz <joze...@mittosystems.com> wrote: > > On Fri, 5 Jul 2019 11:26:20 +0200 > Christophe Lyon <christophe.l...@linaro.org> wrote: > > > On Thu, 4 Jul 2019 at 23:46, Jozef Lawrynowicz <joze...@mittosystems.com> > > wrote: > > > > > > Also, the gcc.target/arm/noinit-attribute.c test works with msp430. > > > Why not create a effective-target keyword which checks for noinit > > > support, so > > > the test can be gated on that condition and put in a generic part of the > > > testsuite? > > > > That was my intention, and I was waiting for feedback on this. In this > > case, I suppose the effective-target check would test a hardcoded list > > of targets, like many other effective-targets? > > (eg check_weak_available) > > Were there previous discussions on whether the noinit attribute should be > explicitly enabled for certain targets? e.g. so it will error if you try to > use > it on x86_64.
Not formally. I submitted a patch for arm only https://gcc.gnu.org/ml/gcc-patches/2019-06/msg00771.html and got requests to make it generic instead, starting with: https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00065.html > If it will just be enabled by default for all targets then a hardcoded > list for check-effective target sounds ok. Otherwise if it does cause an error > when used on an unsupported target you could do a check that the > attribute compiles successfully instead. Currently, I think it will be accepted for all targets, with various effects depending on the OS etc... > > > > > After doing some further testing, I noticed the attribute does not work on > > > static variables. The attribute handling code allows the attribute to be > > > set on > > > a static variable, but then that variable does not get placed in the > > > .noinit > > > section. > > > > > > What are your thoughts on this? > > > > > > Does it even makes sense for a static local variable to be declared as > > > "noinit"? > > > One should want a static local variable to be initialized, as having it > > > not > > > initialized and hold a random value could cause problems. > > > Perhaps a user could think of some use for this, but I can't. > > > > > I added: > > static int __attribute__((noinit)) var_noinit2; > > in global scope, and > > static int __attribute__((noinit)) var_noinit3; > > in main(), and the generate code contains: > > .section .noinit,"aw" > > .align 2 > > .set .LANCHOR2,. + 0 > > .type var_noinit2, %object > > .size var_noinit2, 4 > > var_noinit2: > > .space 4 > > .type var_noinit3.4160, %object > > .size var_noinit3.4160, 4 > > var_noinit3.4160: > > .space 4 > > .type var_noinit, %object > > .size var_noinit, 4 > > var_noinit: > > .space 4 > > > > So, all three of them are in the .noinit section, but only var_noinit has > > .global var_noinit > > > > So it seems to work? > > Hmm yes there seems to be an issue with trunks handling of noinit for msp430. > Even before your patch the static noinit variables are marked as > common symbols with ".comm" and are not placed in .noinit. > > These static noinit variables work with my downstream msp430-gcc (which > doesn't > yet have parity with upstream), so this is just something I'll fix separately, > and doesn't represent any issues with your patch. > > Jozef
