* __packed vs. __attribute__((packed)) in kernel headers
@ 2011-06-22 6:34 Markus Trippelsdorf
2011-06-23 13:42 ` Nick Bowler
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Markus Trippelsdorf @ 2011-06-22 6:34 UTC (permalink / raw)
To: linux-kernel@vger.kernel.org
Cc: Mike Frysinger, Sam Ravnborg, Artem Bityutskiy
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)):
diff --git a/scripts/headers_install.pl b/scripts/headers_install.pl
index efb3be1..e0dc065 100644
--- a/scripts/headers_install.pl
+++ b/scripts/headers_install.pl
@@ -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?
--
Markus
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: __packed vs. __attribute__((packed)) in kernel headers 2011-06-22 6:34 __packed vs. __attribute__((packed)) in kernel headers Markus Trippelsdorf @ 2011-06-23 13:42 ` Nick Bowler 2011-06-23 15:02 ` Markus Trippelsdorf 2011-06-23 17:04 ` __packed vs. __attribute__((packed)) in " richard -rw- weinberger 2011-06-23 17:54 ` Mike Frysinger 2 siblings, 1 reply; 21+ messages in thread From: Nick Bowler @ 2011-06-23 13:42 UTC (permalink / raw) To: Markus Trippelsdorf Cc: linux-kernel@vger.kernel.org, Mike Frysinger, Sam Ravnborg, Artem Bityutskiy On 2011-06-22 08:34 +0200, Markus Trippelsdorf wrote: > One possible solution would be to let the "scripts/headers_install.pl" > script automatically substitute __packed with __attribute__((packed)): > > diff --git a/scripts/headers_install.pl b/scripts/headers_install.pl > index efb3be1..e0dc065 100644 > --- a/scripts/headers_install.pl > +++ b/scripts/headers_install.pl > @@ -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? 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, -- Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: __packed vs. __attribute__((packed)) in kernel headers 2011-06-23 13:42 ` Nick Bowler @ 2011-06-23 15:02 ` Markus Trippelsdorf 2011-06-23 16:57 ` Joe Perches 0 siblings, 1 reply; 21+ messages in thread From: Markus Trippelsdorf @ 2011-06-23 15:02 UTC (permalink / raw) To: Nick Bowler Cc: linux-kernel@vger.kernel.org, Mike Frysinger, Sam Ravnborg, Artem Bityutskiy On 2011.06.23 at 09:42 -0400, Nick Bowler wrote: > On 2011-06-22 08:34 +0200, Markus Trippelsdorf wrote: > > One possible solution would be to let the "scripts/headers_install.pl" > > script automatically substitute __packed with __attribute__((packed)): > > > > diff --git a/scripts/headers_install.pl b/scripts/headers_install.pl > > index efb3be1..e0dc065 100644 > > --- a/scripts/headers_install.pl > > +++ b/scripts/headers_install.pl > > @@ -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? > > 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. 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? -- Markus ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: __packed vs. __attribute__((packed)) in kernel headers 2011-06-23 15:02 ` Markus Trippelsdorf @ 2011-06-23 16:57 ` Joe Perches 2011-06-24 13:07 ` Michal Marek 0 siblings, 1 reply; 21+ messages in thread From: Joe Perches @ 2011-06-23 16:57 UTC (permalink / raw) To: Markus Trippelsdorf Cc: Nick Bowler, linux-kernel@vger.kernel.org, Mike Frysinger, Sam Ravnborg, Artem Bityutskiy On Thu, 2011-06-23 at 17:02 +0200, Markus Trippelsdorf wrote: > On 2011.06.23 at 09:42 -0400, Nick Bowler wrote: > > On 2011-06-22 08:34 +0200, Markus Trippelsdorf wrote: > > > One possible solution would be to let the "scripts/headers_install.pl" > > > script automatically substitute __packed with __attribute__((packed)): > > > > > > diff --git a/scripts/headers_install.pl b/scripts/headers_install.pl [] > 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: __packed vs. __attribute__((packed)) in kernel headers 2011-06-23 16:57 ` Joe Perches @ 2011-06-24 13:07 ` Michal Marek 2011-06-24 13:51 ` {PATCH] fix __packed in exported " Markus Trippelsdorf 0 siblings, 1 reply; 21+ messages in thread From: Michal Marek @ 2011-06-24 13:07 UTC (permalink / raw) To: Joe Perches Cc: Markus Trippelsdorf, Nick Bowler, linux-kernel@vger.kernel.org, Mike Frysinger, Sam Ravnborg, Artem Bityutskiy On 23.6.2011 18:57, Joe Perches wrote: > On Thu, 2011-06-23 at 17:02 +0200, Markus Trippelsdorf wrote: >> On 2011.06.23 at 09:42 -0400, Nick Bowler wrote: >>> On 2011-06-22 08:34 +0200, Markus Trippelsdorf wrote: >>>> One possible solution would be to let the "scripts/headers_install.pl" >>>> script automatically substitute __packed with __attribute__((packed)): >>>> >>>> diff --git a/scripts/headers_install.pl b/scripts/headers_install.pl > [] >> 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? > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* {PATCH] fix __packed in exported kernel headers 2011-06-24 13:07 ` Michal Marek @ 2011-06-24 13:51 ` Markus Trippelsdorf 2011-06-24 15:17 ` Michal Marek 2011-06-24 16:33 ` Arnd Bergmann 0 siblings, 2 replies; 21+ messages in thread From: Markus Trippelsdorf @ 2011-06-24 13:51 UTC (permalink / raw) To: Michal Marek Cc: Joe Perches, Nick Bowler, linux-kernel@vger.kernel.org, Mike Frysinger, Sam Ravnborg, Artem Bityutskiy On 2011.06.24 at 15:07 +0200, Michal Marek wrote: > On 23.6.2011 18:57, Joe Perches wrote: > > On Thu, 2011-06-23 at 17:02 +0200, Markus Trippelsdorf wrote: > >> On 2011.06.23 at 09:42 -0400, Nick Bowler wrote: > >>> On 2011-06-22 08:34 +0200, Markus Trippelsdorf wrote: > >>>> One possible solution would be to let the "scripts/headers_install.pl" > >>>> script automatically substitute __packed with __attribute__((packed)): > >>>> > >>>> diff --git a/scripts/headers_install.pl b/scripts/headers_install.pl > > [] > >> 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? > 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 <Artem.Bityutskiy@nokia.com> CC: Joe Perches <joe@perches.com> Signed-off-by: Markus Trippelsdorf <markus@trippelsdorf.de> --- scripts/headers_install.pl | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/scripts/headers_install.pl b/scripts/headers_install.pl index efb3be1..48462be 100644 --- a/scripts/headers_install.pl +++ b/scripts/headers_install.pl @@ -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; -- Markus ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: {PATCH] fix __packed in exported kernel headers 2011-06-24 13:51 ` {PATCH] fix __packed in exported " Markus Trippelsdorf @ 2011-06-24 15:17 ` Michal Marek 2011-06-24 16:33 ` Arnd Bergmann 1 sibling, 0 replies; 21+ messages in thread From: Michal Marek @ 2011-06-24 15:17 UTC (permalink / raw) To: Markus Trippelsdorf Cc: Joe Perches, Nick Bowler, linux-kernel@vger.kernel.org, Mike Frysinger, Sam Ravnborg, Artem Bityutskiy On 24.6.2011 15:51, Markus Trippelsdorf wrote: > 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<Artem.Bityutskiy@nokia.com> > CC: Joe Perches<joe@perches.com> > Signed-off-by: Markus Trippelsdorf<markus@trippelsdorf.de> > --- > scripts/headers_install.pl | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) Thanks, applied to kbuild-2.6.git#kbuild. Michal ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: {PATCH] fix __packed in exported kernel headers 2011-06-24 13:51 ` {PATCH] fix __packed in exported " Markus Trippelsdorf 2011-06-24 15:17 ` Michal Marek @ 2011-06-24 16:33 ` Arnd Bergmann 2011-06-24 17:00 ` Mike Frysinger 2011-06-24 17:01 ` Markus Trippelsdorf 1 sibling, 2 replies; 21+ messages in thread From: Arnd Bergmann @ 2011-06-24 16:33 UTC (permalink / raw) To: Markus Trippelsdorf Cc: Michal Marek, Joe Perches, Nick Bowler, linux-kernel@vger.kernel.org, Mike Frysinger, Sam Ravnborg, Artem Bityutskiy On Friday 24 June 2011, Markus Trippelsdorf wrote: > 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. 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: {PATCH] fix __packed in exported kernel headers 2011-06-24 16:33 ` Arnd Bergmann @ 2011-06-24 17:00 ` Mike Frysinger 2011-06-30 18:26 ` H. Peter Anvin 2011-06-24 17:01 ` Markus Trippelsdorf 1 sibling, 1 reply; 21+ messages in thread From: Mike Frysinger @ 2011-06-24 17:00 UTC (permalink / raw) To: Arnd Bergmann Cc: Markus Trippelsdorf, Michal Marek, Joe Perches, Nick Bowler, linux-kernel@vger.kernel.org, Sam Ravnborg, Artem Bityutskiy On Fri, Jun 24, 2011 at 12:33, Arnd Bergmann wrote: > On Friday 24 June 2011, Markus Trippelsdorf wrote: >> 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. > > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: {PATCH] fix __packed in exported kernel headers 2011-06-24 17:00 ` Mike Frysinger @ 2011-06-30 18:26 ` H. Peter Anvin 2011-06-30 18:48 ` Mike Frysinger 0 siblings, 1 reply; 21+ messages in thread From: H. Peter Anvin @ 2011-06-30 18:26 UTC (permalink / raw) To: Mike Frysinger Cc: Arnd Bergmann, Markus Trippelsdorf, Michal Marek, Joe Perches, Nick Bowler, linux-kernel@vger.kernel.org, Sam Ravnborg, Artem Bityutskiy On 06/24/2011 10:00 AM, Mike Frysinger wrote: >> >> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: {PATCH] fix __packed in exported kernel headers 2011-06-30 18:26 ` H. Peter Anvin @ 2011-06-30 18:48 ` Mike Frysinger 2011-06-30 18:52 ` H. Peter Anvin 0 siblings, 1 reply; 21+ messages in thread From: Mike Frysinger @ 2011-06-30 18:48 UTC (permalink / raw) To: H. Peter Anvin Cc: Arnd Bergmann, Markus Trippelsdorf, Michal Marek, Joe Perches, Nick Bowler, linux-kernel@vger.kernel.org, Sam Ravnborg, Artem Bityutskiy On Thu, Jun 30, 2011 at 14:26, H. Peter Anvin wrote: > On 06/24/2011 10:00 AM, Mike Frysinger wrote: >>> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: {PATCH] fix __packed in exported kernel headers 2011-06-30 18:48 ` Mike Frysinger @ 2011-06-30 18:52 ` H. Peter Anvin 2011-06-30 18:58 ` Mike Frysinger 0 siblings, 1 reply; 21+ messages in thread From: H. Peter Anvin @ 2011-06-30 18:52 UTC (permalink / raw) To: Mike Frysinger Cc: Arnd Bergmann, Markus Trippelsdorf, Michal Marek, Joe Perches, Nick Bowler, linux-kernel@vger.kernel.org, Sam Ravnborg, Artem Bityutskiy On 06/30/2011 11:48 AM, Mike Frysinger wrote: > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: {PATCH] fix __packed in exported kernel headers 2011-06-30 18:52 ` H. Peter Anvin @ 2011-06-30 18:58 ` Mike Frysinger 2011-06-30 19:01 ` H. Peter Anvin 0 siblings, 1 reply; 21+ messages in thread From: Mike Frysinger @ 2011-06-30 18:58 UTC (permalink / raw) To: H. Peter Anvin Cc: Arnd Bergmann, Markus Trippelsdorf, Michal Marek, Joe Perches, Nick Bowler, linux-kernel@vger.kernel.org, Sam Ravnborg, Artem Bityutskiy On Thu, Jun 30, 2011 at 14:52, H. Peter Anvin wrote: > On 06/30/2011 11:48 AM, Mike Frysinger wrote: >> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: {PATCH] fix __packed in exported kernel headers 2011-06-30 18:58 ` Mike Frysinger @ 2011-06-30 19:01 ` H. Peter Anvin 2011-06-30 19:13 ` Mike Frysinger 0 siblings, 1 reply; 21+ messages in thread From: H. Peter Anvin @ 2011-06-30 19:01 UTC (permalink / raw) To: Mike Frysinger Cc: Arnd Bergmann, Markus Trippelsdorf, Michal Marek, Joe Perches, Nick Bowler, linux-kernel@vger.kernel.org, Sam Ravnborg, Artem Bityutskiy On 06/30/2011 11:58 AM, Mike Frysinger wrote: > On Thu, Jun 30, 2011 at 14:52, H. Peter Anvin wrote: >> On 06/30/2011 11:48 AM, Mike Frysinger wrote: >>> 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. > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: {PATCH] fix __packed in exported kernel headers 2011-06-30 19:01 ` H. Peter Anvin @ 2011-06-30 19:13 ` Mike Frysinger 2011-06-30 20:02 ` H. Peter Anvin 0 siblings, 1 reply; 21+ messages in thread From: Mike Frysinger @ 2011-06-30 19:13 UTC (permalink / raw) To: H. Peter Anvin Cc: Arnd Bergmann, Markus Trippelsdorf, Michal Marek, Joe Perches, Nick Bowler, linux-kernel@vger.kernel.org, Sam Ravnborg, Artem Bityutskiy On Thu, Jun 30, 2011 at 15:01, H. Peter Anvin wrote: > On 06/30/2011 11:58 AM, Mike Frysinger wrote: >> On Thu, Jun 30, 2011 at 14:52, H. Peter Anvin wrote: >>> On 06/30/2011 11:48 AM, Mike Frysinger wrote: >>>> 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. > > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: {PATCH] fix __packed in exported kernel headers 2011-06-30 19:13 ` Mike Frysinger @ 2011-06-30 20:02 ` H. Peter Anvin 2011-06-30 21:56 ` Mike Frysinger 0 siblings, 1 reply; 21+ messages in thread From: H. Peter Anvin @ 2011-06-30 20:02 UTC (permalink / raw) To: Mike Frysinger Cc: Arnd Bergmann, Markus Trippelsdorf, Michal Marek, Joe Perches, Nick Bowler, linux-kernel@vger.kernel.org, Sam Ravnborg, Artem Bityutskiy On 06/30/2011 12:13 PM, Mike Frysinger wrote: >> >> 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: {PATCH] fix __packed in exported kernel headers 2011-06-30 20:02 ` H. Peter Anvin @ 2011-06-30 21:56 ` Mike Frysinger 0 siblings, 0 replies; 21+ messages in thread From: Mike Frysinger @ 2011-06-30 21:56 UTC (permalink / raw) To: H. Peter Anvin Cc: Arnd Bergmann, Markus Trippelsdorf, Michal Marek, Joe Perches, Nick Bowler, linux-kernel@vger.kernel.org, Sam Ravnborg, Artem Bityutskiy On Thu, Jun 30, 2011 at 16:02, H. Peter Anvin wrote: > On 06/30/2011 12:13 PM, Mike Frysinger wrote: >>> 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. > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: {PATCH] fix __packed in exported kernel headers 2011-06-24 16:33 ` Arnd Bergmann 2011-06-24 17:00 ` Mike Frysinger @ 2011-06-24 17:01 ` Markus Trippelsdorf 1 sibling, 0 replies; 21+ messages in thread From: Markus Trippelsdorf @ 2011-06-24 17:01 UTC (permalink / raw) To: Arnd Bergmann Cc: Michal Marek, Joe Perches, Nick Bowler, linux-kernel@vger.kernel.org, Mike Frysinger, Sam Ravnborg, Artem Bityutskiy On 2011.06.24 at 18:33 +0200, Arnd Bergmann wrote: > On Friday 24 June 2011, Markus Trippelsdorf wrote: > > 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. > > 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 -- Markus ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: __packed vs. __attribute__((packed)) in kernel headers 2011-06-22 6:34 __packed vs. __attribute__((packed)) in kernel headers Markus Trippelsdorf 2011-06-23 13:42 ` Nick Bowler @ 2011-06-23 17:04 ` richard -rw- weinberger 2011-06-23 17:46 ` Markus Trippelsdorf 2011-06-23 17:54 ` Mike Frysinger 2 siblings, 1 reply; 21+ messages in thread From: richard -rw- weinberger @ 2011-06-23 17:04 UTC (permalink / raw) To: Markus Trippelsdorf Cc: linux-kernel@vger.kernel.org, Mike Frysinger, Sam Ravnborg, Artem Bityutskiy On Wed, Jun 22, 2011 at 8:34 AM, Markus Trippelsdorf <markus@trippelsdorf.de> wrote: > 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)): > > diff --git a/scripts/headers_install.pl b/scripts/headers_install.pl > index efb3be1..e0dc065 100644 > --- a/scripts/headers_install.pl > +++ b/scripts/headers_install.pl > @@ -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? > Hmm, this introduces a new error source. Whenever we change the definition of __packed we have to adjust headers_install.pl too. -- Thanks, //richard ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: __packed vs. __attribute__((packed)) in kernel headers 2011-06-23 17:04 ` __packed vs. __attribute__((packed)) in " richard -rw- weinberger @ 2011-06-23 17:46 ` Markus Trippelsdorf 0 siblings, 0 replies; 21+ messages in thread From: Markus Trippelsdorf @ 2011-06-23 17:46 UTC (permalink / raw) To: richard -rw- weinberger Cc: linux-kernel@vger.kernel.org, Mike Frysinger, Sam Ravnborg, Artem Bityutskiy On 2011.06.23 at 19:04 +0200, richard -rw- weinberger wrote: > On Wed, Jun 22, 2011 at 8:34 AM, Markus Trippelsdorf > <markus@trippelsdorf.de> wrote: > > 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)): > > > > diff --git a/scripts/headers_install.pl b/scripts/headers_install.pl > > index efb3be1..e0dc065 100644 > > --- a/scripts/headers_install.pl > > +++ b/scripts/headers_install.pl > > @@ -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? > > > > 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. -- Markus ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: __packed vs. __attribute__((packed)) in kernel headers 2011-06-22 6:34 __packed vs. __attribute__((packed)) in kernel headers Markus Trippelsdorf 2011-06-23 13:42 ` Nick Bowler 2011-06-23 17:04 ` __packed vs. __attribute__((packed)) in " richard -rw- weinberger @ 2011-06-23 17:54 ` Mike Frysinger 2 siblings, 0 replies; 21+ messages in thread From: Mike Frysinger @ 2011-06-23 17:54 UTC (permalink / raw) To: Markus Trippelsdorf Cc: linux-kernel@vger.kernel.org, Sam Ravnborg, Artem Bityutskiy On Wed, Jun 22, 2011 at 02:34, Markus Trippelsdorf wrote: > 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2011-06-30 21:56 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-06-22 6:34 __packed vs. __attribute__((packed)) in kernel headers Markus Trippelsdorf
2011-06-23 13:42 ` Nick Bowler
2011-06-23 15:02 ` Markus Trippelsdorf
2011-06-23 16:57 ` Joe Perches
2011-06-24 13:07 ` Michal Marek
2011-06-24 13:51 ` {PATCH] fix __packed in exported " Markus Trippelsdorf
2011-06-24 15:17 ` Michal Marek
2011-06-24 16:33 ` Arnd Bergmann
2011-06-24 17:00 ` Mike Frysinger
2011-06-30 18:26 ` H. Peter Anvin
2011-06-30 18:48 ` Mike Frysinger
2011-06-30 18:52 ` H. Peter Anvin
2011-06-30 18:58 ` Mike Frysinger
2011-06-30 19:01 ` H. Peter Anvin
2011-06-30 19:13 ` Mike Frysinger
2011-06-30 20:02 ` H. Peter Anvin
2011-06-30 21:56 ` Mike Frysinger
2011-06-24 17:01 ` Markus Trippelsdorf
2011-06-23 17:04 ` __packed vs. __attribute__((packed)) in " richard -rw- weinberger
2011-06-23 17:46 ` Markus Trippelsdorf
2011-06-23 17:54 ` Mike Frysinger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox