Unfortunately, not all compilers assumes the structures within a pack
region also need be packed (e.g. metag), so need add a pack explicitly
to satisfy all compilers.
The related error (under metag with allmodconfig):
CC [M] drivers/staging/lustre/lustre/lov/lov_pack.o
drivers/staging/lustre/lustre/lov/lov_pack.c: In function 'lov_getstripe':
drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: duplicate case value
drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: previously used here
And originally, all related code used "__attribute__((packed))", so
still use it instead of '__packed'.
Signed-off-by: Chen Gang <[email protected]>
---
drivers/staging/lustre/lustre/include/lustre/lustre_user.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_user.h b/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
index 6b6c0240..0828b31 100644
--- a/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
+++ b/drivers/staging/lustre/lustre/include/lustre/lustre_user.h
@@ -345,7 +345,7 @@ struct lov_user_md_v3 { /* LOV EA user data (host-endian) */
* lmm_objects, use when writing */
__u16 lmm_layout_gen; /* layout generation number
* used when reading */
- };
+ } __attribute__((packed));
char lmm_pool_name[LOV_MAXPOOLNAME]; /* pool name */
struct lov_user_ost_data_v1 lmm_objects[0]; /* per-stripe data */
} __attribute__((packed));
--
1.7.11.7
On Sat, Jan 18, 2014 at 05:50:34PM +0800, Chen Gang wrote:
> Unfortunately, not all compilers assumes the structures within a pack
> region also need be packed (e.g. metag), so need add a pack explicitly
> to satisfy all compilers.
>
> The related error (under metag with allmodconfig):
>
> CC [M] drivers/staging/lustre/lustre/lov/lov_pack.o
> drivers/staging/lustre/lustre/lov/lov_pack.c: In function 'lov_getstripe':
> drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: duplicate case value
> drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: previously used here
>
> And originally, all related code used "__attribute__((packed))", so
> still use it instead of '__packed'.
Use __packed. Then at least one line will be correct which is better
than nothing.
regards,
dan carpenter
On 01/18/2014 06:05 PM, Dan Carpenter wrote:
> On Sat, Jan 18, 2014 at 05:50:34PM +0800, Chen Gang wrote:
>> Unfortunately, not all compilers assumes the structures within a pack
>> region also need be packed (e.g. metag), so need add a pack explicitly
>> to satisfy all compilers.
>>
>> The related error (under metag with allmodconfig):
>>
>> CC [M] drivers/staging/lustre/lustre/lov/lov_pack.o
>> drivers/staging/lustre/lustre/lov/lov_pack.c: In function 'lov_getstripe':
>> drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: duplicate case value
>> drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: previously used here
>>
>> And originally, all related code used "__attribute__((packed))", so
>> still use it instead of '__packed'.
>
> Use __packed. Then at least one line will be correct which is better
> than nothing.
>
Hmm... but that will break the 'consistency' (which is not quite good
for readers).
For me, it will be better to provide another patch to change all
"__attribute__((packed))" to "__packed" within this file.
What about your ideas?
Thanks.
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
On Sat, Jan 18, 2014 at 06:26:10PM +0800, Chen Gang wrote:
> On 01/18/2014 06:05 PM, Dan Carpenter wrote:
> > On Sat, Jan 18, 2014 at 05:50:34PM +0800, Chen Gang wrote:
> >> Unfortunately, not all compilers assumes the structures within a pack
> >> region also need be packed (e.g. metag), so need add a pack explicitly
> >> to satisfy all compilers.
> >>
> >> The related error (under metag with allmodconfig):
> >>
> >> CC [M] drivers/staging/lustre/lustre/lov/lov_pack.o
> >> drivers/staging/lustre/lustre/lov/lov_pack.c: In function 'lov_getstripe':
> >> drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: duplicate case value
> >> drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: previously used here
> >>
> >> And originally, all related code used "__attribute__((packed))", so
> >> still use it instead of '__packed'.
> >
> > Use __packed. Then at least one line will be correct which is better
> > than nothing.
> >
>
> Hmm... but that will break the 'consistency' (which is not quite good
> for readers).
>
> For me, it will be better to provide another patch to change all
> "__attribute__((packed))" to "__packed" within this file.
>
> What about your ideas?
>
In the end, it's not something we care about enough to ask you to redo
the patch. But what I'm saying is that you should prefer kernel style
over local style. We'll fix the surrounding lines later.
regards,
dan carpenter
On 01/18/2014 10:24 PM, Dan Carpenter wrote:
> On Sat, Jan 18, 2014 at 06:26:10PM +0800, Chen Gang wrote:
>> On 01/18/2014 06:05 PM, Dan Carpenter wrote:
>>> On Sat, Jan 18, 2014 at 05:50:34PM +0800, Chen Gang wrote:
>>>> Unfortunately, not all compilers assumes the structures within a pack
>>>> region also need be packed (e.g. metag), so need add a pack explicitly
>>>> to satisfy all compilers.
>>>>
>>>> The related error (under metag with allmodconfig):
>>>>
>>>> CC [M] drivers/staging/lustre/lustre/lov/lov_pack.o
>>>> drivers/staging/lustre/lustre/lov/lov_pack.c: In function 'lov_getstripe':
>>>> drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: duplicate case value
>>>> drivers/staging/lustre/lustre/lov/lov_pack.c:630: error: previously used here
>>>>
>>>> And originally, all related code used "__attribute__((packed))", so
>>>> still use it instead of '__packed'.
>>>
>>> Use __packed. Then at least one line will be correct which is better
>>> than nothing.
>>>
>>
>> Hmm... but that will break the 'consistency' (which is not quite good
>> for readers).
>>
>> For me, it will be better to provide another patch to change all
>> "__attribute__((packed))" to "__packed" within this file.
>>
>> What about your ideas?
>>
>
> In the end, it's not something we care about enough to ask you to redo
> the patch. But what I'm saying is that you should prefer kernel style
> over local style. We'll fix the surrounding lines later.
>
OK, thanks, and also thank James and "./scripts/checkpatch.pl" who/which
also mentioned about it to me.
BTW: this patch is related with another patch which is discussing (so I
have cc that patch to you and Greg too): "if we could sure that it is a
compiler's feature issue, we will skip this patch".
Thanks
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
Hi Chen,
On 19/01/14 10:07, Chen Gang wrote:
> BTW: this patch is related with another patch which is discussing (so I
> have cc that patch to you and Greg too): "if we could sure that it is a
> compiler's feature issue, we will skip this patch".
If you're referring to the #pragma pack portability issue then this
issue is unrelated since it doesn't use #pragma pack.
The issue is *not* that the compiler is defectively failing to pack
nested structs. Doing that would be utterly broken since it would change
the layout of the same struct depending on where it is placed in the
program, Consider this example (which uses a nested struct rather than
union to demonstrate the point):
struct a {
struct b {
unsigned int x;
unsigned short y;
} x;
unsigned short y;
} __packed;
Both ABIs behave the same here:
Arch sizeof(struct b) sizeof(struct a)
x86_64 8 10
metag 8 10
If struct b is made __packed, again both ABIs behave differently in the
same way:
Arch sizeof(struct b) sizeof(struct a)
x86_64 6 8
metag 6 8
The issue is that C compiler ABIs may (and unfortunately metag ABI does)
pack structs and unions to at least 4 bytes, even if no members of the
struct or union are that large, which means that the nested struct/union
should be __packed too to portably ensure it's the expected size.
Cheers
James
Ah. From so metag is a new arch and not a compiler like the changelog
says.
On Mon, Jan 20, 2014 at 11:56:47AM +0000, James Hogan wrote:
> struct a {
> struct b {
> unsigned int x;
> unsigned short y;
> } x;
> unsigned short y;
> } __packed;
This is not the code we are discussing. It should look like:
struct a {
union {
short x;
short y;
}
short z;
};
Any normal person would assume that sizeof(struct a) would be 4 but
apparently on metag it is 8. That totally defeats the point of using
a union in the first place. It's easy enough to add a __packed to the
lustre declaration but I expect this to cause an endless stream of bugs.
It it is really stupid.
regards,
dan carpenter
On 20/01/14 12:30, Dan Carpenter wrote:
> Ah. From so metag is a new arch and not a compiler like the changelog
> says.
>
> On Mon, Jan 20, 2014 at 11:56:47AM +0000, James Hogan wrote:
>> struct a {
>> struct b {
>> unsigned int x;
>> unsigned short y;
>> } x;
>> unsigned short y;
>> } __packed;
>
> This is not the code we are discussing. It should look like:
>
> struct a {
> union {
> short x;
> short y;
> }
> short z;
> };
>
> Any normal person would assume that sizeof(struct a) would be 4 but
> apparently on metag it is 8. That totally defeats the point of using
> a union in the first place. It's easy enough to add a __packed to the
> lustre declaration but I expect this to cause an endless stream of bugs.
>
> It it is really stupid.
I agree completely (and did request this be changed when I first found
out about it, but since it's an ABI issue it was really too late).
That's why I'm not actively pushing for every case to be fixed unless
it's in generic code that actually affects metag.
Cheers
James
On Mon, Jan 20, 2014 at 12:37:57PM +0000, James Hogan wrote:
> On 20/01/14 12:30, Dan Carpenter wrote:
> > Ah. From so metag is a new arch and not a compiler like the changelog
> > says.
> >
> > On Mon, Jan 20, 2014 at 11:56:47AM +0000, James Hogan wrote:
> >> struct a {
> >> struct b {
> >> unsigned int x;
> >> unsigned short y;
> >> } x;
> >> unsigned short y;
> >> } __packed;
> >
> > This is not the code we are discussing. It should look like:
> >
> > struct a {
> > union {
> > short x;
> > short y;
> > }
> > short z;
> > };
> >
> > Any normal person would assume that sizeof(struct a) would be 4 but
> > apparently on metag it is 8. That totally defeats the point of using
> > a union in the first place. It's easy enough to add a __packed to the
> > lustre declaration but I expect this to cause an endless stream of bugs.
> >
> > It it is really stupid.
>
> I agree completely (and did request this be changed when I first found
> out about it, but since it's an ABI issue it was really too late).
> That's why I'm not actively pushing for every case to be fixed unless
> it's in generic code that actually affects metag.
>
It would be easy enough to make the compiler complain about any union
which would normally have size which is not a multiple of 4.
Warning: union will be padded with 2 bytes unless __attribute__((packed)).
Otherwise you will be fighting this for ever.
regards,
dan carpenter
Are you sure it's padding the unions, and not just treating the unions
as structs? What is the size of this union?
union a {
int x;
short y;
};
regards,
dan carpenter
On 20/01/14 13:01, Dan Carpenter wrote:
> It would be easy enough to make the compiler complain about any union
> which would normally have size which is not a multiple of 4.
>
> Warning: union will be padded with 2 bytes unless __attribute__((packed)).
>
> Otherwise you will be fighting this for ever.
A good idea, but the problem is that most of the time it just doesn't
matter since all users of the data structure do so with the same ABI. We
already expect the compiler to take some liberties with padding and
alignment since the C standard permits it, so it's only when the exact
layout really matters (data structures dealing with hardware, stored to
disk, or passed over networks) that it ever becomes a problem.
> Are you sure it's padding the unions, and not just treating the unions
> as structs?
Yes
> What is the size of this union?
>
> union a {
> int x;
> short y;
> };
4
Thanks
James
I made a quick and dirty sparse patch to check for this. I don't think
I will bother trying to send it to sparse upstream, but you can if you
want to.
It found 289 unions which might need a __packed added. The lustre
unions were not in my allmodconfig so they're not listed.
Perhaps there could be a command line option or a pragma so that unions
will work in the kernel. We don't care about linking to outside
libraries.
regards,
dan carpenter
diff --git a/symbol.c b/symbol.c
index ebba56deaf94..596e47883aad 100644
--- a/symbol.c
+++ b/symbol.c
@@ -187,6 +187,12 @@ static struct symbol * examine_struct_union_type(struct symbol *sym, int advance
bit_size = (bit_size + bit_align) & ~bit_align;
}
sym->bit_size = bit_size;
+
+ if (!advance && (info.bit_size / 8) % 4) {
+ int pad = 4 - (info.bit_size / 8) % 4;
+ warning(sym->pos, "'%s' union will be padded with %d bytes unless __attribute__((packed)).", sym->ident ? sym->ident->name: "<null>", pad);
+ }
+
return sym;
}
Hi Dan,
On 20/01/14 21:13, Dan Carpenter wrote:
> I made a quick and dirty sparse patch to check for this. I don't think
> I will bother trying to send it to sparse upstream, but you can if you
> want to.
>
> It found 289 unions which might need a __packed added. The lustre
> unions were not in my allmodconfig so they're not listed.
Thanks a lot for this, it seems to be useful. I'm adapting it to reduce
false negatives (e.g. omitting the check if the struct/union is already
packed), and I imagine it could be made to only warn about padded
unpacked structs/unions which are used as nested members of packed
structs/unions. It wouldn't catch everything but would probably catch a
lot of cases that are most likely to be genuine since they would have
been packed at the outer level for a reason.
> Perhaps there could be a command line option or a pragma so that unions
> will work in the kernel. We don't care about linking to outside
> libraries.
We still interact with userland via structs and unions, so it would
probably have to exclude anything in uapi/.
Cheers
James
On 01/21/2014 06:36 PM, James Hogan wrote:
> Hi Dan,
>
> On 20/01/14 21:13, Dan Carpenter wrote:
>> I made a quick and dirty sparse patch to check for this. I don't think
>> I will bother trying to send it to sparse upstream, but you can if you
>> want to.
>>
>> It found 289 unions which might need a __packed added. The lustre
>> unions were not in my allmodconfig so they're not listed.
>
> Thanks a lot for this, it seems to be useful. I'm adapting it to reduce
> false negatives (e.g. omitting the check if the struct/union is already
> packed), and I imagine it could be made to only warn about padded
> unpacked structs/unions which are used as nested members of packed
> structs/unions. It wouldn't catch everything but would probably catch a
> lot of cases that are most likely to be genuine since they would have
> been packed at the outer level for a reason.
>
>> Perhaps there could be a command line option or a pragma so that unions
>> will work in the kernel. We don't care about linking to outside
>> libraries.
>
> We still interact with userland via structs and unions, so it would
> probably have to exclude anything in uapi/.
>
Thank all of you firstly.
But excuse me, I am still not quit clear that: "what need we do enough
to solve this feature issue?"
So I guess our current result is:
- It is not a good idea to only let kernel to fit with compiler.
- It is not a good idea to only let compiler to fit with kernel.
- Need let compiler and kernel to fit with each other:
- compiler will print related warning, but not break compiling.
so metag compiler need be improvement (check and warn for it).
- if check alignment explicitly in kernel source code, it need be
fixed within kernel: "apply related patches (pack each struct or
union), but the related patch comments need be improved".
Is what I guess correct?
Thanks.
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
On 01/25/2014 07:55 PM, Chen Gang wrote:
> On 01/21/2014 06:36 PM, James Hogan wrote:
>> Hi Dan,
>>
>> On 20/01/14 21:13, Dan Carpenter wrote:
>>> I made a quick and dirty sparse patch to check for this. I don't think
>>> I will bother trying to send it to sparse upstream, but you can if you
>>> want to.
>>>
>>> It found 289 unions which might need a __packed added. The lustre
>>> unions were not in my allmodconfig so they're not listed.
>>
>> Thanks a lot for this, it seems to be useful. I'm adapting it to reduce
>> false negatives (e.g. omitting the check if the struct/union is already
>> packed), and I imagine it could be made to only warn about padded
>> unpacked structs/unions which are used as nested members of packed
>> structs/unions. It wouldn't catch everything but would probably catch a
>> lot of cases that are most likely to be genuine since they would have
>> been packed at the outer level for a reason.
>>
>>> Perhaps there could be a command line option or a pragma so that unions
>>> will work in the kernel. We don't care about linking to outside
>>> libraries.
>>
>> We still interact with userland via structs and unions, so it would
>> probably have to exclude anything in uapi/.
>>
>
It seems, our kernel still stick to treate 'pack' region have effect
with both 'align' and 'sizeof'.
So for compiler, could we add one additional cflag parameter to tell
compiler to switch it (compatible with ABI, or satisfy upstream kernel).
And for kernel, it will be OK enough to only append this parameter to
KBUILD_CFLAGS in "arch/metag/Makefile".
Thanks.
> Thank all of you firstly.
>
> But excuse me, I am still not quit clear that: "what need we do enough
> to solve this feature issue?"
>
> So I guess our current result is:
>
> - It is not a good idea to only let kernel to fit with compiler.
>
> - It is not a good idea to only let compiler to fit with kernel.
>
> - Need let compiler and kernel to fit with each other:
>
> - compiler will print related warning, but not break compiling.
> so metag compiler need be improvement (check and warn for it).
>
> - if check alignment explicitly in kernel source code, it need be
> fixed within kernel: "apply related patches (pack each struct or
> union), but the related patch comments need be improved".
>
> Is what I guess correct?
>
> Thanks.
>
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
> It seems, our kernel still stick to treate 'pack' region have effect
> with both 'align' and 'sizeof'.
>
It's not about packed regions. It's about unions. It's saying the
sizeof() a union is a multiple of 4 unless it's packed.
union foo {
short x;
short y;
};
The author intended the sizeof(union foo) to be 2 but on metag arch then
it is 4.
regards,
dan carpenter
On 02/03/2014 04:58 PM, Dan Carpenter wrote:
> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
>> It seems, our kernel still stick to treate 'pack' region have effect
>> with both 'align' and 'sizeof'.
>>
>
> It's not about packed regions. It's about unions. It's saying the
> sizeof() a union is a multiple of 4 unless it's packed.
>
> union foo {
> short x;
> short y;
> };
>
> The author intended the sizeof(union foo) to be 2 but on metag arch then
> it is 4.
>
Yeah, just like your original discussion. :-)
Hmm... can we say: "for metag compiler, in a pack region, it considers
variables alignment, but does not consider about struct/union alignment
(except append packed to each related struct/union)".
For compatible (consider about its ABI), it has to keep this features,
but for kernel, it needs be changed.
So, I suggest to add one parameter to compiler to switch this feature,
and append this parameter to KBUILD_CFLAGS in "arch/metag/Makefile"
which can satisfy both ABI and kernel.
Thanks.
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
From: Dan Carpenter
> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
> > It seems, our kernel still stick to treate 'pack' region have effect
> > with both 'align' and 'sizeof'.
>
> It's not about packed regions. It's about unions. It's saying the
> sizeof() a union is a multiple of 4 unless it's packed.
>
> union foo {
> short x;
> short y;
> };
>
> The author intended the sizeof(union foo) to be 2 but on metag arch then
> it is 4.
The same is probably be true of: struct foo { _u16 bar; };
Architectures that define such alignment rules are a right PITA.
You either need to get the size to 2 without using 'packed', or
just not define such structures.
It is worth seeing if adding aligned(2) will change the size - I'm
not sure.
David
On 03/02/14 10:05, David Laight wrote:
> From: Dan Carpenter
>> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
>>> It seems, our kernel still stick to treate 'pack' region have effect
>>> with both 'align' and 'sizeof'.
>>
>> It's not about packed regions. It's about unions. It's saying the
>> sizeof() a union is a multiple of 4 unless it's packed.
>>
>> union foo {
>> short x;
>> short y;
>> };
>>
>> The author intended the sizeof(union foo) to be 2 but on metag arch then
>> it is 4.
>
> The same is probably be true of: struct foo { _u16 bar; };
Yes indeed.
> Architectures that define such alignment rules are a right PITA.
> You either need to get the size to 2 without using 'packed', or
> just not define such structures.
> It is worth seeing if adding aligned(2) will change the size - I'm
> not sure.
__aligned(2) alone doesn't seem to have any effect on sizeof() or
__alignof__() unless it is accompanied by __packed. x86_64 is similar in
that respect (it just packs sanely in the first place).
Combining __packed with __aligned(2) does the trick though (__packed
alone sets __aligned(1) which is obviously going to be suboptimal).
Cheers
James
On 02/03/2014 06:05 PM, David Laight wrote:
> From: Dan Carpenter
>> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
>>> It seems, our kernel still stick to treate 'pack' region have effect
>>> with both 'align' and 'sizeof'.
>>
>> It's not about packed regions. It's about unions. It's saying the
>> sizeof() a union is a multiple of 4 unless it's packed.
>>
>> union foo {
>> short x;
>> short y;
>> };
>>
>> The author intended the sizeof(union foo) to be 2 but on metag arch then
>> it is 4.
>
> The same is probably be true of: struct foo { _u16 bar; };
>
I guess so.
> Architectures that define such alignment rules are a right PITA.
Sorry, I do not know about PITA (after google or wiki, I can not get
more related information).
Could you provide more information about PITA, thanks?
> You either need to get the size to 2 without using 'packed', or
> just not define such structures.
Excuse me, I don't quite understand your meaning. I guess your meaning
is:
"normally, we should not use a struct/union like that, no matter what it is (2 or 4)".
Is it correct.
> It is worth seeing if adding aligned(2) will change the size - I'm
> not sure.
>
Yes, it will/should make sure that it must be 2.
Thanks.
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
On 02/03/2014 06:22 PM, James Hogan wrote:
> On 03/02/14 10:05, David Laight wrote:
>> From: Dan Carpenter
>>> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
>>>> It seems, our kernel still stick to treate 'pack' region have effect
>>>> with both 'align' and 'sizeof'.
>>>
>>> It's not about packed regions. It's about unions. It's saying the
>>> sizeof() a union is a multiple of 4 unless it's packed.
>>>
>>> union foo {
>>> short x;
>>> short y;
>>> };
>>>
>>> The author intended the sizeof(union foo) to be 2 but on metag arch then
>>> it is 4.
>>
>> The same is probably be true of: struct foo { _u16 bar; };
>
> Yes indeed.
>
>> Architectures that define such alignment rules are a right PITA.
>> You either need to get the size to 2 without using 'packed', or
>> just not define such structures.
>> It is worth seeing if adding aligned(2) will change the size - I'm
>> not sure.
>
> __aligned(2) alone doesn't seem to have any effect on sizeof() or
> __alignof__() unless it is accompanied by __packed. x86_64 is similar in
> that respect (it just packs sanely in the first place).
>
> Combining __packed with __aligned(2) does the trick though (__packed
> alone sets __aligned(1) which is obviously going to be suboptimal).
>
Oh, thank you for your explanation.
And hope this feature issue can be fixed, and satisfy both kernel and
ABI. :-)
Thanks.
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
From: James Hogan
> On 03/02/14 10:05, David Laight wrote:
> > From: Dan Carpenter
> >> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
> >>> It seems, our kernel still stick to treate 'pack' region have effect
> >>> with both 'align' and 'sizeof'.
> >>
> >> It's not about packed regions. It's about unions. It's saying the
> >> sizeof() a union is a multiple of 4 unless it's packed.
> >>
> >> union foo {
> >> short x;
> >> short y;
> >> };
> >>
> >> The author intended the sizeof(union foo) to be 2 but on metag arch then
> >> it is 4.
> >
> > The same is probably be true of: struct foo { _u16 bar; };
>
> Yes indeed.
>
> > Architectures that define such alignment rules are a right PITA.
> > You either need to get the size to 2 without using 'packed', or
> > just not define such structures.
> > It is worth seeing if adding aligned(2) will change the size - I'm
> > not sure.
>
> __aligned(2) alone doesn't seem to have any effect on sizeof() or
> __alignof__() unless it is accompanied by __packed. x86_64 is similar in
> that respect (it just packs sanely in the first place).
>
> Combining __packed with __aligned(2) does the trick though (__packed
> alone sets __aligned(1) which is obviously going to be suboptimal).
Compile some code for a cpu that doesn't support misaligned transfers
(probably one of sparc, arm, ppc) and see if the compiler generates a
single 16bit request or two 8 bits ones.
You don't want the compiler generating multiple byte-sized memory transfers.
David
On 03/02/14 10:35, David Laight wrote:
> From: James Hogan
>> On 03/02/14 10:05, David Laight wrote:
>>> Architectures that define such alignment rules are a right PITA.
>>> You either need to get the size to 2 without using 'packed', or
>>> just not define such structures.
>>> It is worth seeing if adding aligned(2) will change the size - I'm
>>> not sure.
>>
>> __aligned(2) alone doesn't seem to have any effect on sizeof() or
>> __alignof__() unless it is accompanied by __packed. x86_64 is similar in
>> that respect (it just packs sanely in the first place).
>>
>> Combining __packed with __aligned(2) does the trick though (__packed
>> alone sets __aligned(1) which is obviously going to be suboptimal).
>
> Compile some code for a cpu that doesn't support misaligned transfers
> (probably one of sparc, arm, ppc) and see if the compiler generates a
> single 16bit request or two 8 bits ones.
> You don't want the compiler generating multiple byte-sized memory transfers.
Meta is also one of those arches, and according to my quick tests,
__packed alone does correctly make it fall back to byte loads/stores,
but with __packed __aligned(2) it uses 16bit loads/stores. I've also
confirmed that with an ARM toolchain (see below for example).
Cheers
James
input:
#define __aligned(x) __attribute__((aligned(x)))
#define __packed __attribute__((packed))
union a {
short x, y;
} __aligned(2) __packed;
struct b {
short x;
} __aligned(2) __packed;
unsigned int soa = sizeof(union a);
unsigned int aoa = __alignof__(union a);
unsigned int sob = sizeof(struct b);
unsigned int aob = __alignof__(struct b);
void t(struct b *x, union a *y)
{
++x->x;
++y->x;
}
ARM output (-O2):
.cpu arm10tdmi
.fpu softvfp
.eabi_attribute 20, 1
.eabi_attribute 21, 1
.eabi_attribute 23, 3
.eabi_attribute 24, 1
.eabi_attribute 25, 1
.eabi_attribute 26, 2
.eabi_attribute 30, 2
.eabi_attribute 34, 0
.eabi_attribute 18, 4
.file "alignment4.c"
.text
.align 2
.global t
.type t, %function
t:
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 0, uses_anonymous_args = 0
@ link register save eliminated.
ldrh r3, [r0, #0]
add r3, r3, #1
strh r3, [r0, #0] @ movhi
ldrh r3, [r1, #0]
add r3, r3, #1
strh r3, [r1, #0] @ movhi
bx lr
.size t, .-t
.global aob
.global sob
.global aoa
.global soa
.data
.align 2
.type aob, %object
.size aob, 4
aob:
.word 2
.type sob, %object
.size sob, 4
sob:
.word 2
.type aoa, %object
.size aoa, 4
aoa:
.word 2
.type soa, %object
.size soa, 4
soa:
.word 2
.ident "GCC: (GNU) 4.7.1 20120606 (Red Hat 4.7.1-0.1.20120606)"
.section .note.GNU-stack,"",%progbits
On 02/03/2014 06:03 PM, Chen Gang wrote:
> On 02/03/2014 04:58 PM, Dan Carpenter wrote:
>> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
>>> It seems, our kernel still stick to treate 'pack' region have effect
>>> with both 'align' and 'sizeof'.
>>>
>>
>> It's not about packed regions. It's about unions. It's saying the
>> sizeof() a union is a multiple of 4 unless it's packed.
>>
>> union foo {
>> short x;
>> short y;
>> };
>>
>> The author intended the sizeof(union foo) to be 2 but on metag arch then
>> it is 4.
>>
>
> Yeah, just like your original discussion. :-)
>
>
> Hmm... can we say: "for metag compiler, in a pack region, it considers
> variables alignment, but does not consider about struct/union alignment
> (except append packed to each related struct/union)".
>
> For compatible (consider about its ABI), it has to keep this features,
> but for kernel, it needs be changed.
>
> So, I suggest to add one parameter to compiler to switch this feature,
> and append this parameter to KBUILD_CFLAGS in "arch/metag/Makefile"
> which can satisfy both ABI and kernel.
>
After append the parameter to KBUILD_CFLAGS in "arch/metag/Makefile",
- I guess/assume "include/uapi/*" should/will not need be modified.
- but need check all files in "arch/metag/include/uapi/*".
(add padding data for packed struct/union when __KERNEL__ defined)
- maybe we have to process metag related ABI which not in "*/uapi/*"
(add padding data for packed struct/union when __KERNEL__ defined)
and when we find them, recommend to move all of them to "*/uapi/*".
Sorry, I don't know whether this way is the best way or not, but for me
it is an executable way to solve this feature issue and satisfy both
kernel and ABI.
Thanks.
--
Chen Gang
Open, share and attitude like air, water and life which God blessed
From: James Hogan
> On 03/02/14 10:35, David Laight wrote:
> > From: James Hogan
> >> Combining __packed with __aligned(2) does the trick though (__packed
> >> alone sets __aligned(1) which is obviously going to be suboptimal).
...
>
> Meta is also one of those arches, and according to my quick tests,
> __packed alone does correctly make it fall back to byte loads/stores,
> but with __packed __aligned(2) it uses 16bit loads/stores. I've also
> confirmed that with an ARM toolchain (see below for example).
I would either:
1a) Add explicit padding to the relevant structures so that they are
multiple of 4 bytes.
or:
1b) #define some token to "__packed __aligned(2)" before all the structures
that require changing, and use that in there definitions.
This lets you comment on WHY you are doing it.
and:
2) Add a compile-time assert that the structures are the correct size.
Clearly you don't want to mark anything that contains a 32bit value
with __packed __aligned(2).
I'm not at all clear whether you are sometimes using a different compiler.
David