Hi David et al.
After improving the headers_check.pl script to do a
check for files included with:
#include "foo.h"
it reported the following error:
/home/sam/kernel/knext.git/usr/include/drm/via_drm.h:34: included file '/home/sam/kernel/knext.git/usr/include/drm/via_drmclient.h' is not exported
And indeed the file via_drmclient.h does not exist in the kernel source.
We do not see any issue in the kernel as the include is guarded by:
#ifndef __KERNEL__
#include "via_drmclient.h"
#endif
But referring to a non-existing file is not the right thing to to.
We need this check as we have several places where we do:
#include "foo.h"
in our exported headers.
So we need to fix drm somehow.
The headers_check.pl patch is included for reference.
Sam
diff --git a/scripts/headers_check.pl b/scripts/headers_check.pl
index 488a3b1..b97341e 100644
--- a/scripts/headers_check.pl
+++ b/scripts/headers_check.pl
@@ -32,7 +32,7 @@ foreach my $file (@files) {
$lineno = 0;
while ($line = <FH>) {
$lineno++;
- check_include();
+ check_include($filename);
}
close FH;
}
@@ -40,7 +40,8 @@ exit $ret;
sub check_include
{
- if ($line =~ m/^\s*#\s*include\s+<((asm|linux).*)>/) {
+ my ($filename) = @_;
+ if ($line =~ m/^\s*#\s*include\s+<((asm|linux)[_a-zA-Z0-9\/.]+)>/) {
my $inc = $1;
my $found;
$found = stat($dir . "/" . $inc);
@@ -52,5 +53,18 @@ sub check_include
printf STDERR "$filename:$lineno: included file '$inc' is not exported\n";
$ret = 1;
}
+ } elsif ($line =~ m/^\s*#\s*include\s+"([_a-zA-Z0-9.]+)"/) {
+ my $inc = $1;
+ my $filedir = $filename;
+ my $found;
+
+ # drop filename part so we have only the dir part
+ $filedir =~ s/\/[_a-zA-Z0-9.]+$//;
+
+ $found = stat($filedir . "/" . $inc);
+ if (!$found) {
+ printf STDERR "$filename:$lineno: included file '$filedir/$inc' is not exported\n";
+ $ret = 1;
+ }
}
}
Hi!
The intention is for via_drm.h to be self-containing when included both
for a kernel build and for a user-space build. In this particular case,
via_drmclient.h lives in the user-space clients and includes stdint.h to
get access to uint32_t and friends.
Of course, the user-space clients could
#include "uint32.h"
#include "via_drm.h"
but shouldn't really the tools be mimicing what the compiler does in
this case?
/Thomas
Sam Ravnborg wrote:
> Hi David et al.
>
> After improving the headers_check.pl script to do a
> check for files included with:
>
> #include "foo.h"
>
> it reported the following error:
>
> /home/sam/kernel/knext.git/usr/include/drm/via_drm.h:34: included file '/home/sam/kernel/knext.git/usr/include/drm/via_drmclient.h' is not exported
>
> And indeed the file via_drmclient.h does not exist in the kernel source.
> We do not see any issue in the kernel as the include is guarded by:
>
> #ifndef __KERNEL__
> #include "via_drmclient.h"
> #endif
>
> But referring to a non-existing file is not the right thing to to.
>
> We need this check as we have several places where we do:
>
> #include "foo.h"
>
> in our exported headers.
>
> So we need to fix drm somehow.
>
> The headers_check.pl patch is included for reference.
>
>
On Sun, Dec 14, 2008 at 04:57:34PM +0100, Thomas Hellstr?m wrote:
> Hi!
>
> The intention is for via_drm.h to be self-containing when included both
> for a kernel build and for a user-space build. In this particular case,
> via_drmclient.h lives in the user-space clients and includes stdint.h to
> get access to uint32_t and friends.
>
> Of course, the user-space clients could
> #include "uint32.h"
> #include "via_drm.h"
>
> but shouldn't really the tools be mimicing what the compiler does in
> this case?
The kernel headers and thus the kernel ABI is separate and ideally
they should not depend on any other header files to provide anything.
This is why __u32, __u64 etc are preferred in the kernel ABI
and not uint32_t as used by the drm headers.
We do not adhere to this as a strict rule (yet).
But if you do:
grep -l uint32_t usr/include/linux
then you will only see 7 hits. Out of 368 files.
So we are not bad in this respect.
For drm the fix seems simple - just replace all of uint32_t with __u32.
likewise for the other 32 bit and the 64 bit variants.
For the specific case where drm includes a non-existing file I suggest
that we get this fixed in some way soon.
Sam
Hi!
Sam Ravnborg wrote:
> On Sun, Dec 14, 2008 at 04:57:34PM +0100, Thomas Hellstr?m wrote:
>
>> Hi!
>>
>> The intention is for via_drm.h to be self-containing when included both
>> for a kernel build and for a user-space build. In this particular case,
>> via_drmclient.h lives in the user-space clients and includes stdint.h to
>> get access to uint32_t and friends.
>>
>> Of course, the user-space clients could
>> #include "uint32.h"
>> #include "via_drm.h"
>>
>> but shouldn't really the tools be mimicing what the compiler does in
>> this case?
>>
>
> The kernel headers and thus the kernel ABI is separate and ideally
> they should not depend on any other header files to provide anything.
>
> This is why __u32, __u64 etc are preferred in the kernel ABI
> and not uint32_t as used by the drm headers.
>
> We do not adhere to this as a strict rule (yet).
> But if you do:
>
> grep -l uint32_t usr/include/linux
>
> then you will only see 7 hits. Out of 368 files.
> So we are not bad in this respect.
>
> For drm the fix seems simple - just replace all of uint32_t with __u32.
> likewise for the other 32 bit and the 64 bit variants.
>
I'm not sure that's possible, as these header files are shared with *BSD
(and possibly Solaris).
> For the specific case where drm includes a non-existing file I suggest
> that we get this fixed in some way soon.
>
It's a bit non-trivial, since all via drm user-space clients need to be
fixed, but definitiely doable.
/Thomas
On Mon, Dec 15, 2008 at 10:26:27PM +0100, Thomas Hellstr?m wrote:
> Hi!
>
> Sam Ravnborg wrote:
> >On Sun, Dec 14, 2008 at 04:57:34PM +0100, Thomas Hellstr?m wrote:
> >
> >>Hi!
> >>
> >>The intention is for via_drm.h to be self-containing when included both
> >>for a kernel build and for a user-space build. In this particular case,
> >>via_drmclient.h lives in the user-space clients and includes stdint.h to
> >>get access to uint32_t and friends.
> >>
> >>Of course, the user-space clients could
> >>#include "uint32.h"
> >>#include "via_drm.h"
> >>
> >>but shouldn't really the tools be mimicing what the compiler does in
> >>this case?
> >>
> >
> >The kernel headers and thus the kernel ABI is separate and ideally
> >they should not depend on any other header files to provide anything.
> >
> >This is why __u32, __u64 etc are preferred in the kernel ABI
> >and not uint32_t as used by the drm headers.
> >
> >We do not adhere to this as a strict rule (yet).
> >But if you do:
> >
> >grep -l uint32_t usr/include/linux
> >
> >then you will only see 7 hits. Out of 368 files.
> >So we are not bad in this respect.
> >
> >For drm the fix seems simple - just replace all of uint32_t with __u32.
> >likewise for the other 32 bit and the 64 bit variants.
> >
> I'm not sure that's possible, as these header files are shared with *BSD
> (and possibly Solaris).
I dunno about *BSD and solaris.
But other linux ABI header files do:
#include <linux/types.h>
To get access to these basic types.
And we could do the same for drm if the other systems does not prevent it.
Sam