[RFC/PATCH 1/2] sparse: Fix including glibc headers on x86-64

[RFC/PATCH 1/2] sparse: Fix including glibc headers on x86-64

[penberg]

From: Pekka Enberg <penberg@kernel.org>

Sparse treats code as 32-bit which causes problems for userspace projects on
x86-64 Fedora if you don't have the 32-bit glibc compat package installed
("glib-devel.i686"):

  $ ./sparse allocate.c
  /usr/include/gnu/stubs.h:7:12: error: unable to open 'gnu/stubs-32.h'

Fix the issue by defining relevant macros on 64-bit.

Cc: Christopher Li <sparse@chrisli.org>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
---
 lib.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/lib.c b/lib.c
index 396e9f1..f372296 100644
--- a/lib.c
+++ b/lib.c
@@ -827,6 +827,12 @@ void create_builtin_stream(void)
 	add_pre_buffer("#weak_define __LONG_MAX__ " STRINGIFY(__LONG_MAX__) "\n");
 	add_pre_buffer("#weak_define __LONG_LONG_MAX__ " STRINGIFY(__LONG_LONG_MAX__) "\n");
 	add_pre_buffer("#weak_define __WCHAR_MAX__ " STRINGIFY(__WCHAR_MAX__) "\n");
+
+#ifdef __x86_64
+	add_pre_buffer("#weak_define x86_64 1\n");
+	add_pre_buffer("#weak_define __x86_64 1\n");
+	add_pre_buffer("#weak_define __x86_64__ 1\n");
+#endif
 }
 
 static struct symbol_list *sparse_tokenstream(struct token *token)
-- 
1.7.6.4

[RFC/PATCH 2/2] sparse: Use native sizes for data types

[penberg]

From: Pekka Enberg <penberg@kernel.org>

This patch is needed to fix the sparsec LLVM backend data type sizes.

Cc: Christopher Li <sparse@chrisli.org>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Pekka Enberg <penberg@kernel.org>
---
 target.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/target.c b/target.c
index 6a535bc..009002f 100644
--- a/target.c
+++ b/target.c
@@ -17,9 +17,9 @@ int max_alignment = 16;
 int bits_in_bool = 8;
 int bits_in_char = 8;
 int bits_in_short = 16;
-int bits_in_int = 32;
-int bits_in_long = 32;
-int bits_in_longlong = 64;
+int bits_in_int = sizeof(int) * 8;
+int bits_in_long = sizeof(long) * 8;
+int bits_in_longlong = sizeof(long long) * 8;
 int bits_in_longlonglong = 128;
 
 int max_int_alignment = 4;
@@ -36,8 +36,8 @@ int max_fp_alignment = 8;
 /*
  * Pointer data type
  */
-int bits_in_pointer = 32;
-int pointer_alignment = 4;
+int bits_in_pointer = sizeof(void *) * 8;
+int pointer_alignment = sizeof(void *); 
 
 /*
  * Enum data types
-- 
1.7.6.4

Re: [RFC/PATCH 2/2] sparse: Use native sizes for data types

[Jeff Garzik]

On 10/23/2011 08:37 AM, penberg@cs.helsinki.fi wrote:
> From: Pekka Enberg<penberg@kernel.org>
>
> This patch is needed to fix the sparsec LLVM backend data type sizes.
>
> Cc: Christopher Li<sparse@chrisli.org>
> Cc: Jeff Garzik<jgarzik@redhat.com>
> Cc: Linus Torvalds<torvalds@linux-foundation.org>
> Signed-off-by: Pekka Enberg<penberg@kernel.org>
> ---
>   target.c |   10 +++++-----
>   1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/target.c b/target.c
> index 6a535bc..009002f 100644
> --- a/target.c
> +++ b/target.c
> @@ -17,9 +17,9 @@ int max_alignment = 16;
>   int bits_in_bool = 8;
>   int bits_in_char = 8;
>   int bits_in_short = 16;
> -int bits_in_int = 32;
> -int bits_in_long = 32;
> -int bits_in_longlong = 64;
> +int bits_in_int = sizeof(int) * 8;
> +int bits_in_long = sizeof(long) * 8;
> +int bits_in_longlong = sizeof(long long) * 8;
>   int bits_in_longlonglong = 128;
>
>   int max_int_alignment = 4;
> @@ -36,8 +36,8 @@ int max_fp_alignment = 8;
>   /*
>    * Pointer data type
>    */
> -int bits_in_pointer = 32;
> -int pointer_alignment = 4;
> +int bits_in_pointer = sizeof(void *) * 8;
> +int pointer_alignment = sizeof(void *);

No objection, but ideally we should select from a target template.

We don't want to start down the road of making runtime target switching 
(i386/x86-64) difficult.

	Jeff

Re: [RFC/PATCH 2/2] sparse: Use native sizes for data types

[Josh Triplett]

On Sun, Oct 23, 2011 at 11:53:00AM -0400, Jeff Garzik wrote:
> On 10/23/2011 08:37 AM, penberg@cs.helsinki.fi wrote:
> >From: Pekka Enberg<penberg@kernel.org>
> >
> >This patch is needed to fix the sparsec LLVM backend data type sizes.
> >
> >Cc: Christopher Li<sparse@chrisli.org>
> >Cc: Jeff Garzik<jgarzik@redhat.com>
> >Cc: Linus Torvalds<torvalds@linux-foundation.org>
> >Signed-off-by: Pekka Enberg<penberg@kernel.org>
> >---
> >  target.c |   10 +++++-----
> >  1 files changed, 5 insertions(+), 5 deletions(-)
> >
> >diff --git a/target.c b/target.c
> >index 6a535bc..009002f 100644
> >--- a/target.c
> >+++ b/target.c
> >@@ -17,9 +17,9 @@ int max_alignment = 16;
> >  int bits_in_bool = 8;
> >  int bits_in_char = 8;
> >  int bits_in_short = 16;
> >-int bits_in_int = 32;
> >-int bits_in_long = 32;
> >-int bits_in_longlong = 64;
> >+int bits_in_int = sizeof(int) * 8;
> >+int bits_in_long = sizeof(long) * 8;
> >+int bits_in_longlong = sizeof(long long) * 8;
> >  int bits_in_longlonglong = 128;
> >
> >  int max_int_alignment = 4;
> >@@ -36,8 +36,8 @@ int max_fp_alignment = 8;
> >  /*
> >   * Pointer data type
> >   */
> >-int bits_in_pointer = 32;
> >-int pointer_alignment = 4;
> >+int bits_in_pointer = sizeof(void *) * 8;
> >+int pointer_alignment = sizeof(void *);
> 
> No objection, but ideally we should select from a target template.
> 
> We don't want to start down the road of making runtime target
> switching (i386/x86-64) difficult.

Agreed.  The platform that sparse checks code for should not necessarily
correlate with what sparse observes at compile-time about the build
platform.

- Josh Triplett

Re: [RFC/PATCH 2/2] sparse: Use native sizes for data types

[Pekka Enberg]

On Sun, Oct 23, 2011 at 8:13 PM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Sun, Oct 23, 2011 at 11:53:00AM -0400, Jeff Garzik wrote:
>> On 10/23/2011 08:37 AM, penberg@cs.helsinki.fi wrote:
>> >From: Pekka Enberg<penberg@kernel.org>
>> >
>> >This patch is needed to fix the sparsec LLVM backend data type sizes.
>> >
>> >Cc: Christopher Li<sparse@chrisli.org>
>> >Cc: Jeff Garzik<jgarzik@redhat.com>
>> >Cc: Linus Torvalds<torvalds@linux-foundation.org>
>> >Signed-off-by: Pekka Enberg<penberg@kernel.org>
>> >---
>> >  target.c |   10 +++++-----
>> >  1 files changed, 5 insertions(+), 5 deletions(-)
>> >
>> >diff --git a/target.c b/target.c
>> >index 6a535bc..009002f 100644
>> >--- a/target.c
>> >+++ b/target.c
>> >@@ -17,9 +17,9 @@ int max_alignment = 16;
>> >  int bits_in_bool = 8;
>> >  int bits_in_char = 8;
>> >  int bits_in_short = 16;
>> >-int bits_in_int = 32;
>> >-int bits_in_long = 32;
>> >-int bits_in_longlong = 64;
>> >+int bits_in_int = sizeof(int) * 8;
>> >+int bits_in_long = sizeof(long) * 8;
>> >+int bits_in_longlong = sizeof(long long) * 8;
>> >  int bits_in_longlonglong = 128;
>> >
>> >  int max_int_alignment = 4;
>> >@@ -36,8 +36,8 @@ int max_fp_alignment = 8;
>> >  /*
>> >   * Pointer data type
>> >   */
>> >-int bits_in_pointer = 32;
>> >-int pointer_alignment = 4;
>> >+int bits_in_pointer = sizeof(void *) * 8;
>> >+int pointer_alignment = sizeof(void *);
>>
>> No objection, but ideally we should select from a target template.
>>
>> We don't want to start down the road of making runtime target
>> switching (i386/x86-64) difficult.
>
> Agreed.  The platform that sparse checks code for should not necessarily
> correlate with what sparse observes at compile-time about the build
> platform.

Sure, I'm happy to implement this in some other way if you have an
idea how to do it cleanly. My patches don't make runtime switching
harder and I think using native data type sizes for plain sparse is a
better default.

                        Pekka
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 1/2] sparse: Fix including glibc headers on x86-64

[Christopher Li]

On Sun, Oct 23, 2011 at 5:36 AM,  <penberg@cs.helsinki.fi> wrote:
> From: Pekka Enberg <penberg@kernel.org>
>
> Sparse treats code as 32-bit which causes problems for userspace projects on
> x86-64 Fedora if you don't have the 32-bit glibc compat package installed
> ("glib-devel.i686"):

A blast from the past. Thanks Frederic for reminding me this one.

> +
> +#ifdef __x86_64
> +       add_pre_buffer("#weak_define x86_64 1\n");
> +       add_pre_buffer("#weak_define __x86_64 1\n");
> +       add_pre_buffer("#weak_define __x86_64__ 1\n");
> +#endif
>  }

I just realized that this patch has the unwanted side effect that,
even if you try to define 32 bit compile in 64 bit Linux, the __x86_64__ will
still be there. The weak define only avoid the conflict of defining
the __x86_64__
the second time. It does not solve the case where __x86_64__ shouldn't
be there.

The better way seems to be parsing -m32 and -m64 then set the
__x86_64__ accordingly.

Chris

Re: [RFC/PATCH 1/2] sparse: Fix including glibc headers on x86-64

[Benjamin Herrenschmidt]

On Fri, 2012-03-23 at 01:59 -0700, Christopher Li wrote:
> 
> I just realized that this patch has the unwanted side effect that,
> even if you try to define 32 bit compile in 64 bit Linux, the
> __x86_64__ will
> still be there. The weak define only avoid the conflict of defining
> the __x86_64__
> the second time. It does not solve the case where __x86_64__ shouldn't
> be there.
> 
> The better way seems to be parsing -m32 and -m64 then set the
> __x86_64__ accordingly. 

That reminds me we should also have a way to pass the arch triplet and
have a good way to find a "default". On debian sid you need it to get
all your standard includes for userspace, and we should really pass it
to llvm in sparse-llvm.

Cheers,
Ben.

Re: [RFC/PATCH 1/2] sparse: Fix including glibc headers on x86-64

[Christopher Li]

On Fri, Mar 23, 2012 at 2:01 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> That reminds me we should also have a way to pass the arch triplet and
> have a good way to find a "default". On debian sid you need it to get
> all your standard includes for userspace, and we should really pass it
> to llvm in sparse-llvm.

I agree. The right direction to go is to have some arch parsing.
If we have those arch parsing, we don't need the cgcc script any more.

Converting all arch is pretty involved. I don't have a good way to test
non x86 platform.

We can start from -m32 parsing and work our way through the arch parsing.

Chris

Re: [RFC/PATCH 1/2] sparse: Fix including glibc headers on x86-64

[Christopher Li]

[-- Attachment #1: Type: text/plain, Size: 262 bytes --]

Here is the patch I am purposing.

Please let me know if I handle the default size parsing wrong.

Chris

On Fri, Mar 23, 2012 at 2:13 AM, Christopher Li <sparse@chrisli.org> wrote:
>
> We can start from -m32 parsing and work our way through the arch parsing.
>

[-- Attachment #2: 0001-Adding-default-for-m64-m32-handle.patch --]
[-- Type: text/x-patch, Size: 2520 bytes --]

From 95e614e382aaee4b6d1229aa8e9955eabb3f4756 Mon Sep 17 00:00:00 2001
From: Christopher Li <sparse@chrisli.org>
Date: Fri, 23 Mar 2012 02:58:20 -0700
Subject: [PATCH] Adding default for m64/m32 handle

This is improved version of Pekka Enberg's patch
"Fix including glibc headers on x86-64".

To avoid setting 64 bit define in the -m32 case,
the 64 bit initialization needs to delay until all
arguments are handled.

Signed-off-by: Christopher Li <sparse@chrisli.org>
---
 lib.c |   43 +++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/lib.c b/lib.c
index 396e9f1..fc06018 100644
--- a/lib.c
+++ b/lib.c
@@ -224,6 +224,15 @@ static enum { STANDARD_C89,
               STANDARD_GNU89,
               STANDARD_GNU99, } standard = STANDARD_GNU89;
 
+#ifdef __x86_64__
+#define ARCH_M64_DEFAULT 1
+#else
+#define ARCH_M64_DEFAULT 0
+#endif
+
+int arch_m64 = ARCH_M64_DEFAULT;
+int arch_msize_long = 0;
+
 #define CMDLINE_INCLUDE 20
 int cmdline_include_nr = 0;
 struct cmdline_include cmdline_include[CMDLINE_INCLUDE];
@@ -344,19 +353,47 @@ static char **handle_switch_M(char *arg, char **next)
 static char **handle_switch_m(char *arg, char **next)
 {
 	if (!strcmp(arg, "m64")) {
+		arch_m64 = 1;
+	} else if (!strcmp(arg, "m32")) {
+		arch_m64 = 0;
+	} else if (!strcmp(arg, "msize-long")) {
+		arch_msize_long = 1;
+	}
+	return next;
+}
+
+static void handle_arch_m64_finalize(void)
+{
+	if (arch_m64) {
 		bits_in_long = 64;
 		max_int_alignment = 8;
 		bits_in_pointer = 64;
 		pointer_alignment = 8;
 		size_t_ctype = &ulong_ctype;
 		ssize_t_ctype = &long_ctype;
-	} else if (!strcmp(arg, "msize-long")) {
+#ifdef __x86_64__
+		add_pre_buffer("#weak_define x86_64 1\n");
+		add_pre_buffer("#weak_define __x86_64 1\n");
+		add_pre_buffer("#weak_define __x86_64__ 1\n");
+#endif
+	}
+}
+
+static void handle_arch_msize_long_finalize(void)
+{
+	if (arch_msize_long) {
 		size_t_ctype = &ulong_ctype;
 		ssize_t_ctype = &long_ctype;
 	}
-	return next;
 }
 
+static void handle_arch_finalize(void)
+{
+	handle_arch_m64_finalize();
+	handle_arch_msize_long_finalize();
+}
+
+
 static char **handle_switch_o(char *arg, char **next)
 {
 	if (!strcmp (arg, "o")) {       // "-o foo"
@@ -931,6 +968,8 @@ struct symbol_list *sparse_initialize(int argc, char **argv, struct string_list
 	handle_switch_W_finalize();
 	handle_switch_v_finalize();
 
+	handle_arch_finalize();
+
 	list = NULL;
 	if (!ptr_list_empty(filelist)) {
 		// Initialize type system
-- 
1.7.7.6

Re: [RFC/PATCH 1/2] sparse: Fix including glibc headers on x86-64

[Josh Triplett]

On Fri, Mar 23, 2012 at 03:03:51AM -0700, Christopher Li wrote:
> --- a/lib.c
> +++ b/lib.c
> @@ -224,6 +224,15 @@ static enum { STANDARD_C89,
>                STANDARD_GNU89,
>                STANDARD_GNU99, } standard = STANDARD_GNU89;
>  
> +#ifdef __x86_64__
> +#define ARCH_M64_DEFAULT 1
> +#else
> +#define ARCH_M64_DEFAULT 0
> +#endif

Ideally this should work on other 64-bit architectures as well.

Other than that, the rest of the patch looks reasonable.

- Josh Triplett

Re: [RFC/PATCH 1/2] sparse: Fix including glibc headers on x86-64

[Christopher Li]

On Fri, Mar 23, 2012 at 11:03 AM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Fri, Mar 23, 2012 at 03:03:51AM -0700, Christopher Li wrote:
>> --- a/lib.c
>> +++ b/lib.c
>> @@ -224,6 +224,15 @@ static enum { STANDARD_C89,
>>                STANDARD_GNU89,
>>                STANDARD_GNU99, } standard = STANDARD_GNU89;
>>
>> +#ifdef __x86_64__
>> +#define ARCH_M64_DEFAULT 1
>> +#else
>> +#define ARCH_M64_DEFAULT 0
>> +#endif
>
> Ideally this should work on other 64-bit architectures as well.


Thanks for the review.

I totally agree.  In the case it did not get it right,
it is not worse than existing code so that is not too bad.
I don't have other 64bit platform to test with though so
I am afraid to change some thing I can't test.

It would be a nice to have follow up patch adding different architecture
handling using the handle_arch_finalize(). As it is, it will
still be wrong if you use sparse as a cross compiler.
We need proper arch handling for that.

Patch are definitely welcome.

Chris
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 1/2] sparse: Fix including glibc headers on x86-64

[Christopher Li]

On Fri, Mar 23, 2012 at 11:29 AM, Christopher Li <sparse@chrisli.org> wrote:
>
> Thanks for the review.

Applied and pushed, along with merging the sparse-llvm again.

Chris