packed vs. attribute__((packed)) in kernel headers

PATCH Markus Trippelsdorf 8 participants 21 messages Jun 22, 2011 – Jun 30, 2011

Share & tools

Markus Trippelsdorf

author

2011-06-22 06:34 UTC #0 lore raw

A recent commit 3627924acf70a changed __attribute__ ((packed)) to __packed in some UBI headers. This breaks the build of busybox:

CC miscutils/ubi_attach_detach.o
In file included from miscutils/ubi_attach_detach.c:27:0:
/usr/include/mtd/ubi-user.h:330:3: error: conflicting types for ‘__packed’ /usr/include/mtd/ubi-user.h:314:3: note: previous declaration of ‘__packed’ was here /usr/include/mtd/ubi-user.h:372:3: error: conflicting types for ‘__packed’ /usr/include/mtd/ubi-user.h:314:3: note: previous declaration of ‘__packed’ was here
...

But this kind of change is suggested by checkpatch.pl:
WARN("__packed is preferred over __attribute__((packed))\n

One possible solution would be to let the "scripts/headers_install.pl" script automatically substitute __packed with __attribute__((packed)):

scripts/headers_install.pl +1 -0

index efb3be1..e0dc065 100644
@@ -35,6 +35,7 @@ foreach my $file (@files) {
 		$line =~ s/([\s(])__iomem\s/$1/g;
 		$line =~ s/\s__attribute_const__\s/ /g;
 		$line =~ s/\s__attribute_const__$//g;
+		$line =~ s/\s__packed/__attribute__((packed))/g;
 		$line =~ s/^#include <linux\/compiler.h>//;
 		$line =~ s/(^|\s)(inline)\b/$1__$2__/g;
 		$line =~ s/(^|\s)(asm)\b(\s|[(]|$)/$1__$2__$3/g;

Any thoughts?

Signature · 1 line

Markus

Nick Bowler

2011-06-23 13:42 UTC #1 lore raw

Quoted 16 lines · Markus Trippelsdorf

One possible solution would be to let the "scripts/headers_install.pl" script automatically substitute __packed with __attribute__((packed)):

scripts/headers_install.pl +1 -0

index efb3be1..e0dc065 100644
@@ -35,6 +35,7 @@ foreach my $file (@files) {
 		$line =~ s/([\s(])__iomem\s/$1/g;
 		$line =~ s/\s__attribute_const__\s/ /g;
 		$line =~ s/\s__attribute_const__$//g;
+		$line =~ s/\s__packed/__attribute__((packed))/g;
 		$line =~ s/^#include <linux\/compiler.h>//;
 		$line =~ s/(^|\s)(inline)\b/$1__$2__/g;
 		$line =~ s/(^|\s)(asm)\b(\s|[(]|$)/$1__$2__$3/g;

Quoted 1 line

Any thoughts?

Without any comment on the approach itself, the above change will eat a space character before __packed, which may break the header by merging a preceding token into the __attribute__. It may also change instances of __packed that occur as a prefix of a longer token.

Note that the __attribute_const__ lines above it take some care to avoid both of these situations.

Cheers,

Signature · 1 line

Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

Markus Trippelsdorf

author

2011-06-23 15:02 UTC #2 lore raw

Quoted 24 lines · Nick Bowler

Quoted 16 lines · Markus Trippelsdorf
One possible solution would be to let the "scripts/headers_install.pl" script automatically substitute __packed with __attribute__((packed)):
scripts/headers_install.pl +1 -0
index efb3be1..e0dc065 100644
@@ -35,6 +35,7 @@ foreach my $file (@files) {
 		$line =~ s/([\s(])__iomem\s/$1/g;
 		$line =~ s/\s__attribute_const__\s/ /g;
 		$line =~ s/\s__attribute_const__$//g;
+		$line =~ s/\s__packed/__attribute__((packed))/g;
 		$line =~ s/^#include <linux\/compiler.h>//;
 		$line =~ s/(^|\s)(inline)\b/$1__$2__/g;
 		$line =~ s/(^|\s)(asm)\b(\s|[(]|$)/$1__$2__$3/g;
Quoted 1 line
Any thoughts?
Without any comment on the approach itself, the above change will eat a space character before __packed, which may break the header by merging a preceding token into the __attribute__. It may also change instances of __packed that occur as a prefix of a longer token.

Quoted 2 lines

Note that the __attribute_const__ lines above it take some care to avoid both of these situations.

Yes, you're right. But the patch was meant as a rough proof of concept and not as the final implementation.

I'm not an expert of Perl regular expressions, but maybe this:
$line =~ s/\s__packed;$/ __attribute__((packed));/g
is a little bit closer to the intention?

Signature · 1 line

Markus

Joe Perches

2011-06-23 16:57 UTC #3 lore raw

Quoted 6 lines · Markus Trippelsdorf

Quoted 5 lines · Nick Bowler
scripts/headers_install.pl
Quoted 4 lines · Markus Trippelsdorf
One possible solution would be to let the "scripts/headers_install.pl" script automatically substitute __packed with __attribute__((packed)):
scripts/headers_install.pl +0 -0

[]

Quoted 3 lines

I'm not an expert of Perl regular expressions, but maybe this:
$line =~ s/\s__packed;$/ __attribute__((packed));/g
is a little bit closer to the intention?

Maybe:

s/\b__packed\b/__attribute__((packed))/g

though this argues against redefining
gcc __attributes__ in the first place.

Michal Marek

2011-06-24 13:07 UTC #4 lore raw

Quoted 8 lines · Joe Perches

Quoted 6 lines · Markus Trippelsdorf
Quoted 5 lines · Nick Bowler
scripts/headers_install.pl
Quoted 4 lines · Markus Trippelsdorf
One possible solution would be to let the "scripts/headers_install.pl" script automatically substitute __packed with __attribute__((packed)):
scripts/headers_install.pl +0 -0
[]

Quoted 7 lines

Quoted 3 lines
I'm not an expert of Perl regular expressions, but maybe this:
$line =~ s/\s__packed;$/ __attribute__((packed));/g
is a little bit closer to the intention?
Maybe:
s/\b__packed\b/__attribute__((packed))/g

Markus, will you post a patch with this fix?

Quoted 2 lines

though this argues against redefining
gcc __attributes__ in the first place.

It's a handy shortcut, so why not have it. Although I don't understand why checkpatch.pl has to warn about __attribute__((packed)).

Michal

Markus Trippelsdorf

author

2011-06-24 13:51 UTC #5 lore raw

{PATCH] fix __packed in exported kernel headers

Quoted 13 lines · Michal Marek

Quoted 12 lines · Joe Perches
Quoted 6 lines · Markus Trippelsdorf
Quoted 5 lines · Nick Bowler
scripts/headers_install.pl
Quoted 4 lines · Markus Trippelsdorf
One possible solution would be to let the "scripts/headers_install.pl" script automatically substitute __packed with __attribute__((packed)):
scripts/headers_install.pl +0 -0
[]
Quoted 3 lines
I'm not an expert of Perl regular expressions, but maybe this:
$line =~ s/\s__packed;$/ __attribute__((packed));/g
is a little bit closer to the intention?

Quoted 6 lines

Quoted 3 lines
Maybe:
s/\b__packed\b/__attribute__((packed))/g
Markus, will you post a patch with this fix?

checkpatch.pl warns about using __attribute__((packed)) in kernel
headers: "__packed is preferred over __attribute__((packed))". If one follows that advice it could cause problems in the exported header files, because the outside world doesn't know about this shortcut.

For example busybox will fail to compile:
CC miscutils/ubi_attach_detach.o
In file included from miscutils/ubi_attach_detach.c:27:0:
/usr/include/mtd/ubi-user.h:330:3: error: conflicting types for ‘__packed’ /usr/include/mtd/ubi-user.h:314:3: note: previous declaration of ‘__packed’ was here
...

Fix the problem by substituting __packed with __attribute__((packed)) in the header_install.pl script.

Cc Artem Bityutskiy <[email protected]>

CC Joe Perches <[email protected]>

Signed-off-by Markus Trippelsdorf <[email protected]>

---
 scripts/headers_install.pl |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

scripts/headers_install.pl +1 -0

index efb3be1..48462be 100644
@@ -35,6 +35,7 @@ foreach my $file (@files) {
 		$line =~ s/([\s(])__iomem\s/$1/g;
 		$line =~ s/\s__attribute_const__\s/ /g;
 		$line =~ s/\s__attribute_const__$//g;
+		$line =~ s/\b__packed\b/__attribute__((packed))/g;
 		$line =~ s/^#include <linux\/compiler.h>//;
 		$line =~ s/(^|\s)(inline)\b/$1__$2__/g;
 		$line =~ s/(^|\s)(asm)\b(\s|[(]|$)/$1__$2__$3/g;

Signature · 1 line

Markus

Michal Marek

2011-06-24 15:17 UTC #6 lore raw

Re: {PATCH] fix __packed in exported kernel headers

Quoted 15 lines · Markus Trippelsdorf

checkpatch.pl warns about using __attribute__((packed)) in kernel
headers: "__packed is preferred over __attribute__((packed))". If one follows that advice it could cause problems in the exported header files, because the outside world doesn't know about this shortcut.
For example busybox will fail to compile:
CC miscutils/ubi_attach_detach.o
In file included from miscutils/ubi_attach_detach.c:27:0:
/usr/include/mtd/ubi-user.h:330:3: error: conflicting types for ‘__packed’ /usr/include/mtd/ubi-user.h:314:3: note: previous declaration of ‘__packed’ was here
...
Fix the problem by substituting __packed with __attribute__((packed)) in the header_install.pl script.

Quoted 6 lines

Cc Artem Bityutskiy<[email protected]>
CC Joe Perches<[email protected]>
Signed-off-by Markus Trippelsdorf<[email protected]>
---
  scripts/headers_install.pl |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)

Thanks, applied to kbuild-2.6.git#kbuild.

Michal

Arnd Bergmann

2011-06-24 16:34 UTC #7 lore raw

Re: {PATCH] fix __packed in exported kernel headers

In reply to Markus Trippelsdorf

Quoted 12 lines · Markus Trippelsdorf

checkpatch.pl warns about using __attribute__((packed)) in kernel
headers: "__packed is preferred over __attribute__((packed))". If one follows that advice it could cause problems in the exported header files, because the outside world doesn't know about this shortcut.
For example busybox will fail to compile:
CC miscutils/ubi_attach_detach.o
In file included from miscutils/ubi_attach_detach.c:27:0:
/usr/include/mtd/ubi-user.h:330:3: error: conflicting types for ‘__packed’ /usr/include/mtd/ubi-user.h:314:3: note: previous declaration of ‘__packed’ was here
...

Quoted 2 lines

Fix the problem by substituting __packed with __attribute__((packed)) in the header_install.pl script.

No objections to the patch, but it should probably be noted that user visible data structures should ideally not have any attributes, because that requires building all applictions using that interface with gcc.

We generally try to add no such requirements on user applications.

Arnd

Mike Frysinger

2011-06-24 17:01 UTC #8 lore raw

Re: {PATCH] fix __packed in exported kernel headers

Quoted 13 lines · Arnd Bergmann

Quoted 12 lines · Markus Trippelsdorf
checkpatch.pl warns about using __attribute__((packed)) in kernel
headers: "__packed is preferred over __attribute__((packed))". If one follows that advice it could cause problems in the exported header files, because the outside world doesn't know about this shortcut.
For example busybox will fail to compile:
CC miscutils/ubi_attach_detach.o
In file included from miscutils/ubi_attach_detach.c:27:0:
/usr/include/mtd/ubi-user.h:330:3: error: conflicting types for ‘__packed’ /usr/include/mtd/ubi-user.h:314:3: note: previous declaration of ‘__packed’ was here
...

Quoted 8 lines

Quoted 2 lines
Fix the problem by substituting __packed with __attribute__((packed)) in the header_install.pl script.
No objections to the patch, but it should probably be noted that user visible data structures should ideally not have any attributes, because that requires building all applictions using that interface with gcc.
We generally try to add no such requirements on user applications.

the only real solution there is to export the compiler.h headers too

plus, many compilers nowadays (like icc/llvm) tend to include support for common gcc attributes ...
-mike

H. Peter Anvin

2011-06-30 18:33 UTC #9 lore raw

Re: {PATCH] fix __packed in exported kernel headers

Quoted 7 lines · Mike Frysinger

Quoted 2 lines
We generally try to add no such requirements on user applications.
the only real solution there is to export the compiler.h headers too
plus, many compilers nowadays (like icc/llvm) tend to include support for common gcc attributes ...

Either that OR define a set of macros as being expected by the environment of the kernel ABI headers (to be provided by the library.)

-hpa

Mike Frysinger

2011-06-30 18:48 UTC #10 lore raw

Re: {PATCH] fix __packed in exported kernel headers

Quoted 10 lines · H. Peter Anvin

Quoted 6 lines · Mike Frysinger
Quoted 1 line
We generally try to add no such requirements on user applications.
the only real solution there is to export the compiler.h headers too
plus, many compilers nowadays (like icc/llvm) tend to include support for common gcc attributes ...
Either that OR define a set of macros as being expected by the environment of the kernel ABI headers (to be provided by the library.)

without fallback logic (#ifndef xxx...#define xxx...#endif), i think that's throwing an unreasonable amount of requirements onto userspace consumers
-mike

H. Peter Anvin

2011-06-30 18:56 UTC #11 lore raw

Re: {PATCH] fix __packed in exported kernel headers

Quoted 3 lines · Mike Frysinger

without fallback logic (#ifndef xxx...#define xxx...#endif), i think that's throwing an unreasonable amount of requirements onto userspace consumers

Unclear. Too much "smarts" in kernel headers is a constant headache to userspace consumers.

-hpa

Mike Frysinger

2011-06-30 18:58 UTC #12 lore raw

Re: {PATCH] fix __packed in exported kernel headers

Quoted 7 lines · H. Peter Anvin

Quoted 3 lines · Mike Frysinger
without fallback logic (#ifndef xxx...#define xxx...#endif), i think that's throwing an unreasonable amount of requirements onto userspace consumers
Unclear. Too much "smarts" in kernel headers is a constant headache to userspace consumers.

not even being able to include a header without hitting a build failure without first declaring some magic defines (which, realistically, the vast majority of people will be doing exactly the same as they'll be using gcc) is unreasonable. hence my suggestion about compiler.h.
-mike

H. Peter Anvin

2011-06-30 19:05 UTC #13 lore raw

Re: {PATCH] fix __packed in exported kernel headers

Quoted 6 lines · Mike Frysinger

Quoted 5 lines · H. Peter Anvin
Quoted 3 lines · Mike Frysinger
without fallback logic (#ifndef xxx...#define xxx...#endif), i think that's throwing an unreasonable amount of requirements onto userspace consumers

Quoted 9 lines

Quoted 2 lines
Unclear. Too much "smarts" in kernel headers is a constant headache to userspace consumers.
not even being able to include a header without hitting a build failure without first declaring some magic defines (which, realistically, the vast majority of people will be doing exactly the same as they'll be using gcc) is unreasonable. hence my suggestion about compiler.h.

Not unless the kernel uses its own namespace for these defines. The thing is, most libraries have their own macro library for this, and collisions are both likely and bad.

-hpa

Mike Frysinger

2011-06-30 19:13 UTC #14 lore raw

Re: {PATCH] fix __packed in exported kernel headers

Quoted 16 lines · H. Peter Anvin

Quoted 6 lines · Mike Frysinger
Quoted 5 lines · H. Peter Anvin
Quoted 3 lines · Mike Frysinger
without fallback logic (#ifndef xxx...#define xxx...#endif), i think that's throwing an unreasonable amount of requirements onto userspace consumers
Quoted 8 lines
Quoted 2 lines
Unclear. Too much "smarts" in kernel headers is a constant headache to userspace consumers.
not even being able to include a header without hitting a build failure without first declaring some magic defines (which, realistically, the vast majority of people will be doing exactly the same as they'll be using gcc) is unreasonable. hence my suggestion about compiler.h.

Quoted 3 lines

Not unless the kernel uses its own namespace for these defines. The thing is, most libraries have their own macro library for this, and collisions are both likely and bad.

while that's true for exporting compiler.h, namespacing is irrelevant to my requirement -- the headers should have sane/usable defaults. -mike

H. Peter Anvin

2011-06-30 20:04 UTC #15 lore raw

Re: {PATCH] fix __packed in exported kernel headers

Quoted 8 lines · Mike Frysinger

Quoted 4 lines
Not unless the kernel uses its own namespace for these defines. The thing is, most libraries have their own macro library for this, and collisions are both likely and bad.
while that's true for exporting compiler.h, namespacing is irrelevant to my requirement -- the headers should have sane/usable defaults.

I have no idea what "your requirements" are, but as someone who has actually implemented a C library on top of the Linux headers I can tell you it's a very real and relevant issue, and that it is historically one of the biggest problems.

-hpa

Mike Frysinger

2011-06-30 21:56 UTC #16 lore raw

Re: {PATCH] fix __packed in exported kernel headers

Quoted 9 lines · H. Peter Anvin

Quoted 6 lines · Mike Frysinger
Quoted 3 lines
Not unless the kernel uses its own namespace for these defines. The thing is, most libraries have their own macro library for this, and collisions are both likely and bad.
while that's true for exporting compiler.h, namespacing is irrelevant to my requirement -- the headers should have sane/usable defaults.
I have no idea what "your requirements" are

it's what ive already said and what you commented on multiple times. exporting a header that cant be included directly without first declaring some random set of defines is broken.

i'm not referring to some arbitrary set of requirements you may not know.

Quoted 4 lines

but as someone who has
actually implemented a C library on top of the Linux headers I can tell you it's a very real and relevant issue, and that it is historically one of the biggest problems.

no one said otherwise. ive done quite a lot of work with uClibc, so i'm familiar with the historical issues as well.
-mike

Markus Trippelsdorf

author

2011-06-24 17:01 UTC #17 lore raw

Re: {PATCH] fix __packed in exported kernel headers

In reply to Arnd Bergmann

Quoted 13 lines · Arnd Bergmann

Quoted 12 lines · Markus Trippelsdorf
checkpatch.pl warns about using __attribute__((packed)) in kernel
headers: "__packed is preferred over __attribute__((packed))". If one follows that advice it could cause problems in the exported header files, because the outside world doesn't know about this shortcut.
For example busybox will fail to compile:
CC miscutils/ubi_attach_detach.o
In file included from miscutils/ubi_attach_detach.c:27:0:
/usr/include/mtd/ubi-user.h:330:3: error: conflicting types for ‘__packed’ /usr/include/mtd/ubi-user.h:314:3: note: previous declaration of ‘__packed’ was here
...

Quoted 8 lines

Quoted 2 lines
Fix the problem by substituting __packed with __attribute__((packed)) in the header_install.pl script.
No objections to the patch, but it should probably be noted that user visible data structures should ideally not have any attributes, because that requires building all applictions using that interface with gcc.
We generally try to add no such requirements on user applications.

Both icc and clang support __attribute__((packed)).

And a rough count shows:
% grep -R "packed" /usr/src/linux/usr/include | wc -l 262

Signature · 1 line

Markus

richard -rw- weinberger

2011-06-23 17:04 UTC #18 lore raw

In reply to Markus Trippelsdorf

Quoted 30 lines · Markus Trippelsdorf

A recent commit 3627924acf70a changed __attribute__ ((packed)) to __packed in some UBI headers. This breaks the build of busybox:
CC miscutils/ubi_attach_detach.o
In file included from miscutils/ubi_attach_detach.c:27:0:
/usr/include/mtd/ubi-user.h:330:3: error: conflicting types for ‘__packed’ /usr/include/mtd/ubi-user.h:314:3: note: previous declaration of ‘__packed’ was here /usr/include/mtd/ubi-user.h:372:3: error: conflicting types for ‘__packed’ /usr/include/mtd/ubi-user.h:314:3: note: previous declaration of ‘__packed’ was here
...
But this kind of change is suggested by checkpatch.pl:
WARN("__packed is preferred over __attribute__((packed))\n
One possible solution would be to let the "scripts/headers_install.pl" script automatically substitute __packed with __attribute__((packed)):
scripts/headers_install.pl +1 -0
index efb3be1..e0dc065 100644
@@ -35,6 +35,7 @@ foreach my $file (@files) {
                $line =~ s/([\s(])__iomem\s/$1/g;
                $line =~ s/\s__attribute_const__\s/ /g;
                $line =~ s/\s__attribute_const__$//g;
+               $line =~ s/\s__packed/__attribute__((packed))/g;
                $line =~ s/^#include <linux\/compiler.h>//;
                $line =~ s/(^|\s)(inline)\b/$1__$2__/g;
                $line =~ s/(^|\s)(asm)\b(\s|[(]|$)/$1__$2__$3/g;

Quoted 2 lines

Any thoughts?

Hmm, this introduces a new error source. Whenever we change the definition of __packed we have to adjust headers_install.pl too.

Signature · 2 lines

Thanks,
//richard

Markus Trippelsdorf

author

2011-06-23 17:46 UTC #19 lore raw

Quoted 32 lines · richard -rw- weinberger

Quoted 30 lines · Markus Trippelsdorf
A recent commit 3627924acf70a changed __attribute__ ((packed)) to __packed in some UBI headers. This breaks the build of busybox:
CC miscutils/ubi_attach_detach.o
In file included from miscutils/ubi_attach_detach.c:27:0:
/usr/include/mtd/ubi-user.h:330:3: error: conflicting types for ‘__packed’ /usr/include/mtd/ubi-user.h:314:3: note: previous declaration of ‘__packed’ was here /usr/include/mtd/ubi-user.h:372:3: error: conflicting types for ‘__packed’ /usr/include/mtd/ubi-user.h:314:3: note: previous declaration of ‘__packed’ was here
...
But this kind of change is suggested by checkpatch.pl:
WARN("__packed is preferred over __attribute__((packed))\n
One possible solution would be to let the "scripts/headers_install.pl" script automatically substitute __packed with __attribute__((packed)):
scripts/headers_install.pl +1 -0
index efb3be1..e0dc065 100644
@@ -35,6 +35,7 @@ foreach my $file (@files) {
                $line =~ s/([\s(])__iomem\s/$1/g;
                $line =~ s/\s__attribute_const__\s/ /g;
                $line =~ s/\s__attribute_const__$//g;
+               $line =~ s/\s__packed/__attribute__((packed))/g;
                $line =~ s/^#include <linux\/compiler.h>//;
                $line =~ s/(^|\s)(inline)\b/$1__$2__/g;
                $line =~ s/(^|\s)(asm)\b(\s|[(]|$)/$1__$2__$3/g;

Quoted 6 lines

Quoted 2 lines
Any thoughts?
Hmm, this introduces a new error source. Whenever we change the definition of __packed we have to adjust headers_install.pl too.

That definition will never change as long as we use gcc.

The __packed shortcut was introduced 2007 (commit 82ddcb04057). It's use in the exported header files could cause problems, because the outside world doesn't necessarily know about that shortcut.

Signature · 1 line

Markus

Mike Frysinger

2011-06-23 17:55 UTC #20 lore raw

In reply to Markus Trippelsdorf

Quoted 2 lines · Markus Trippelsdorf

One possible solution would be to let the "scripts/headers_install.pl" script automatically substitute __packed with __attribute__((packed)):

until we install compiler.h and friends, this is probably the only way -mike