* __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-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
* 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>
---
| 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
--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 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: {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
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