Problematic __attribute__((section(" "))) and gcc alignment

Problematic __attribute__((section(" "))) and gcc alignment

[Mathieu Desnoyers]

Hi,

I just realized, working on my marker infrastructure, that a lot of 
__attribute__((section(" "))) should probably come along with an
aligned() attribute. Since there are no data structures of size greater
or equal to 32 bytes put in these sections later referred to by
__sectionname_start[] and __sectionname_end[], the problem is never
encountered (AFAIK). But as soon as these structures will reach 32 bytes
in size, things will go ill:

Let's take arch/i386/boot/video.h as an example:

it defines 

struct card_info {
        const char *card_name;
        int (*set_mode)(struct mode_info *mode);
        int (*probe)(void);
        struct mode_info *modes;
        int nmodes;             /* Number of probed modes so far */
        int unsafe;             /* Probing is unsafe, only do after "scan" */
        u16 xmode_first;        /* Unprobed modes to try to call anyway */
        u16 xmode_n;            /* Size of unprobed mode range */
};

Which is 28 bytes in size (so it is ok for now). If one single field is
added, gcc will start aligning this structure on 32 bytes boundaries.
(see http://gcc.gnu.org/ml/gcc-bugs/1999-11/msg00914.html)

We then have
#define __videocard struct card_info __attribute__((section(".videocards")))
extern struct card_info video_cards[], video_cards_end[];

Which instructs gcc to put these structures in the .videocards section.
The linker scripts arch/i386/boot/setup.ld will assign video_cards and
video_cards_end as pointers to the beginning and the end of this
section. video_cards[0] is therefore expected to give the first
structure in the section.

The problem with this is that gcc will align it on 32 bytes boundaries
relative to what it "thinks" is the start of the section, which has
nothing to do with the actual section layout given by the linker script.

Therefore, gcc would add extra padding at the beginning of the
.videocards section if the structures within it would become 32 bytes
long, causing video_cards[0] to point into padding instead of the actual
data structure.

Since a change as simple as adding an element to a data structure should
not have to come with those weird alignment considerations, I think it
would make sense to turn every __attribute__((section(" ... ")))
into __attribute__((section(" ... "), aligned(sizeof(void *)))) to make
sure that gcc will not try to align the structures defined in a section
on a boundary bigger than what it thinks is the section start alignment.

Comments on this proposal are welcome,

Mathieu

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: Problematic __attribute__((section(" "))) and gcc alignment

[Sam Ravnborg]

On Thu, Jun 21, 2007 at 04:32:36PM -0400, Mathieu Desnoyers wrote:
> Hi,
> 
> I just realized, working on my marker infrastructure, that a lot of 
> __attribute__((section(" "))) should probably come along with an
> aligned() attribute. Since there are no data structures of size greater
> or equal to 32 bytes put in these sections later referred to by
> __sectionname_start[] and __sectionname_end[], the problem is never
> encountered (AFAIK). But as soon as these structures will reach 32 bytes
> in size, things will go ill:
> 
> Let's take arch/i386/boot/video.h as an example:
> 
> it defines 
> 
> struct card_info {
>         const char *card_name;
>         int (*set_mode)(struct mode_info *mode);
>         int (*probe)(void);
>         struct mode_info *modes;
>         int nmodes;             /* Number of probed modes so far */
>         int unsafe;             /* Probing is unsafe, only do after "scan" */
>         u16 xmode_first;        /* Unprobed modes to try to call anyway */
>         u16 xmode_n;            /* Size of unprobed mode range */
> };
> 
> Which is 28 bytes in size (so it is ok for now). If one single field is
> added, gcc will start aligning this structure on 32 bytes boundaries.
> (see http://gcc.gnu.org/ml/gcc-bugs/1999-11/msg00914.html)
> 
> We then have
> #define __videocard struct card_info __attribute__((section(".videocards")))
> extern struct card_info video_cards[], video_cards_end[];
> 
> Which instructs gcc to put these structures in the .videocards section.
> The linker scripts arch/i386/boot/setup.ld will assign video_cards and
> video_cards_end as pointers to the beginning and the end of this
> section. video_cards[0] is therefore expected to give the first
> structure in the section.

The linker will align the start of the section to the biggest alignment
required by any member in the section. So gcc should tell the linker
that video_cards needs 32 bytes alignemnt and we are not facing trobles.

BUT this requires that the labels in the linker script file are
correct assigned like this:

 .tracedata : AT(ADDR(.tracedata) - LOAD_OFFSET) {
        __tracedata_start = .;
        *(.tracedata)
        __tracedata_end = .;
  }

If the assignment of __tracedata_start was doen just before the .tracedata
we would not use the alignment imposed by linker and would see the error you describe.

	Sam

Re: Problematic __attribute__((section(" "))) and gcc alignment

[Mathieu Desnoyers]

* Sam Ravnborg (sam@ravnborg.org) wrote:
> On Thu, Jun 21, 2007 at 04:32:36PM -0400, Mathieu Desnoyers wrote:
> > Hi,
> > 
> > I just realized, working on my marker infrastructure, that a lot of 
> > __attribute__((section(" "))) should probably come along with an
> > aligned() attribute. Since there are no data structures of size greater
> > or equal to 32 bytes put in these sections later referred to by
> > __sectionname_start[] and __sectionname_end[], the problem is never
> > encountered (AFAIK). But as soon as these structures will reach 32 bytes
> > in size, things will go ill:
> > 
> > Let's take arch/i386/boot/video.h as an example:
> > 
> > it defines 
> > 
> > struct card_info {
> >         const char *card_name;
> >         int (*set_mode)(struct mode_info *mode);
> >         int (*probe)(void);
> >         struct mode_info *modes;
> >         int nmodes;             /* Number of probed modes so far */
> >         int unsafe;             /* Probing is unsafe, only do after "scan" */
> >         u16 xmode_first;        /* Unprobed modes to try to call anyway */
> >         u16 xmode_n;            /* Size of unprobed mode range */
> > };
> > 
> > Which is 28 bytes in size (so it is ok for now). If one single field is
> > added, gcc will start aligning this structure on 32 bytes boundaries.
> > (see http://gcc.gnu.org/ml/gcc-bugs/1999-11/msg00914.html)
> > 
> > We then have
> > #define __videocard struct card_info __attribute__((section(".videocards")))
> > extern struct card_info video_cards[], video_cards_end[];
> > 
> > Which instructs gcc to put these structures in the .videocards section.
> > The linker scripts arch/i386/boot/setup.ld will assign video_cards and
> > video_cards_end as pointers to the beginning and the end of this
> > section. video_cards[0] is therefore expected to give the first
> > structure in the section.
> 
> The linker will align the start of the section to the biggest alignment
> required by any member in the section. So gcc should tell the linker
> that video_cards needs 32 bytes alignemnt and we are not facing trobles.
> 
> BUT this requires that the labels in the linker script file are
> correct assigned like this:
> 
>  .tracedata : AT(ADDR(.tracedata) - LOAD_OFFSET) {
>         __tracedata_start = .;
>         *(.tracedata)
>         __tracedata_end = .;
>   }
> 
> If the assignment of __tracedata_start was doen just before the .tracedata
> we would not use the alignment imposed by linker and would see the error you describe.
> 

Hi Sam,

I was experiencing problems with my addons to the DATA_DATA macro,
declaring stuff in the .data section. It looked like: 

(vmlinux.lds.h) in -mm :

/* .data section */
#define DATA_DATA                                                       \
        *(.data)                                                        \
        *(.data.init.refok)                                             \
        . = ALIGN(8);                                                  \
        VMLINUX_SYMBOL(__start___markers) = .;                          \
        *(__markers)                                                    \
        VMLINUX_SYMBOL(__stop___markers) = .;

All this is declared within the .data section. However, I could not
declare a different section within this macro, because it is already
placed in a section; i.e.

(arch/i386/vmlinux.lds.S) in -mm :
  . = ALIGN(4096);
  .data : AT(ADDR(.data) - LOAD_OFFSET) {       /* Data */
        DATA_DATA
        CONSTRUCTORS
        } :data

Using . = ALIGN(32); fixed my issue, but I wonder if there would be some
way to express the ".tracedata : AT(ADDR(.tracedata) - LOAD_OFFSET)"
that would automatically take care of alignment within this macro?

Thanks,

Mathieu

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

Re: Problematic __attribute__((section(" "))) and gcc alignment

[Denis Vlasenko]

On Thursday 21 June 2007 21:32, Mathieu Desnoyers wrote:
> Let's take arch/i386/boot/video.h as an example:
> 
> it defines 
> 
> struct card_info {
>         const char *card_name;
>         int (*set_mode)(struct mode_info *mode);
>         int (*probe)(void);
>         struct mode_info *modes;
>         int nmodes;             /* Number of probed modes so far */
>         int unsafe;             /* Probing is unsafe, only do after "scan" */
>         u16 xmode_first;        /* Unprobed modes to try to call anyway */
>         u16 xmode_n;            /* Size of unprobed mode range */
> };
> 
> Which is 28 bytes in size (so it is ok for now). If one single field is
> added, gcc will start aligning this structure on 32 bytes boundaries.
> (see http://gcc.gnu.org/ml/gcc-bugs/1999-11/msg00914.html)
> 
> We then have
> #define __videocard struct card_info __attribute__((section(".videocards")))
> extern struct card_info video_cards[], video_cards_end[];
> 
> Which instructs gcc to put these structures in the .videocards section.
> The linker scripts arch/i386/boot/setup.ld will assign video_cards and
> video_cards_end as pointers to the beginning and the end of this
> section. video_cards[0] is therefore expected to give the first
> structure in the section.
> 
> The problem with this is that gcc will align it on 32 bytes boundaries
> relative to what it "thinks" is the start of the section, which has
> nothing to do with the actual section layout given by the linker script.

The problem is that gcc is too eager to align stuff to some big power of two
upon reaching some irrelevant threshold. Why structures 32 bytes and more
in size should be aligned to 32 bytes (even if they have no doubles
and thus are not planned to be used by SSE code) is beyond me.
Why string literals of 32+ bytes are aligned is (beyond me)^2.

These are reverted in latest gcc (for -Os only):

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=31319

but meanwhile gcc started to align stack to 16 bytes, *unconditionally*:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=32849

I imagine 4K stack people will especially like it.

Apart from being bloaty, this also broke de-facto i386 ABI.
There is a solution which isnt bloaty and doesn't break the ABI.
But it wasn't chosen. :(
--
vda