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
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/)
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
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.
On Wed, Jun 22, 2011 at 8:34 AM, Markus Trippelsdorf
<[email protected]> 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
On 2011.06.23 at 19:04 +0200, richard -rw- weinberger wrote:
> On Wed, Jun 22, 2011 at 8:34 AM, Markus Trippelsdorf
> <[email protected]> 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
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
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
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 <[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(-)
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
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<[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
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
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
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
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
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
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
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
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
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
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
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