Cleanup patches

Cleanup patches

[Igmar Palsenberg]

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

Hi,

See attached patches for a clang cleanup. No functional changes, just making sure it compiles without warnings.



Regards,


	Igmar





[-- Attachment #2: 0001-s-target-cleanup.patch --]
[-- Type: application/octet-stream, Size: 686 bytes --]

From 3fdb12ef8182b8bbe1f3113a55f3bfad4288f432 Mon Sep 17 00:00:00 2001
From: Igmar Palsenberg <igmar@palsenberg.com>
Date: Mon, 6 Jun 2011 16:25:14 +0200
Subject: [PATCH 1/6] Remove CFLAGS and INC from the .s target. Not needed,
 generates warnings with clang.

---
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 5c17642..64003f1 100644
--- a/Makefile
+++ b/Makefile
@@ -63,7 +63,7 @@ include/bits/alltypes.h: include/bits/alltypes.h.sh
 	sh $< > $@
 
 %.o: $(ARCH)/%.s
-	$(CC) $(CFLAGS) $(INC) -c -o $@ $<
+	$(CC) -c -o $@ $<
 
 %.o: %.c $(GENH)
 	$(CC) $(CFLAGS) $(INC) -c -o $@ $<
-- 
1.7.5.2


[-- Attachment #3: 0002-Use-flexible-array-member.patch --]
[-- Type: application/octet-stream, Size: 1038 bytes --]

From 5bd75381f4c002b0381adfeb587be735fc11367c Mon Sep 17 00:00:00 2001
From: Igmar Palsenberg <igmar@palsenberg.com>
Date: Mon, 6 Jun 2011 16:27:16 +0200
Subject: [PATCH 2/6] - Use flexible array member instead of [1].

---
 include/dirent.h    |    2 +-
 src/malloc/malloc.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/dirent.h b/include/dirent.h
index ca000bd..5496be3 100644
--- a/include/dirent.h
+++ b/include/dirent.h
@@ -18,7 +18,7 @@ struct dirent
 	off_t d_off;
 	unsigned short d_reclen;
 	unsigned char d_type;
-	char d_name[1];
+	char d_name[];
 };
 
 #define d_fileno d_ino
diff --git a/src/malloc/malloc.c b/src/malloc/malloc.c
index bc8382e..79db0fa 100644
--- a/src/malloc/malloc.c
+++ b/src/malloc/malloc.c
@@ -16,9 +16,9 @@ void *__mremap(void *, size_t, size_t, int, ...);
 int __madvise(void *, size_t, int);
 
 struct chunk {
-	size_t data[1];
 	struct chunk *next;
 	struct chunk *prev;
+	size_t data[];
 };
 
 struct bin {
-- 
1.7.5.2


[-- Attachment #4: 0003-Use-raise.patch --]
[-- Type: application/octet-stream, Size: 1737 bytes --]

From f7d562be536bca329065acdde5a3fcfc0d123d07 Mon Sep 17 00:00:00 2001
From: Igmar Palsenberg <igmar@palsenberg.com>
Date: Mon, 6 Jun 2011 16:53:13 +0200
Subject: [PATCH 3/6] Use raise(SIGSEGV) instead of the nasty NULL pointer
 dereference

---
 src/malloc/malloc.c  |    6 ++++--
 src/time/__asctime.c |    3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/malloc/malloc.c b/src/malloc/malloc.c
index 79db0fa..fda7004 100644
--- a/src/malloc/malloc.c
+++ b/src/malloc/malloc.c
@@ -395,7 +395,8 @@ void *realloc(void *p, size_t n)
 		size_t oldlen = n0 + extra;
 		size_t newlen = n + extra;
 		/* Crash on realloc of freed chunk */
-		if ((uintptr_t)base < mal.brk) *(char *)0=0;
+		if ((uintptr_t)base < mal.brk)
+			raise(SIGSEGV);
 		if (newlen < PAGE_SIZE && (new = malloc(n))) {
 			memcpy(new, p, n-OVERHEAD);
 			free(p);
@@ -458,7 +459,8 @@ void free(void *p)
 		char *base = (char *)self - extra;
 		size_t len = CHUNK_SIZE(self) + extra;
 		/* Crash on double free */
-		if ((uintptr_t)base < mal.brk) *(char *)0=0;
+		if ((uintptr_t)base < mal.brk)
+			raise(SIGSEGV);
 		__munmap(base, len);
 		return;
 	}
diff --git a/src/time/__asctime.c b/src/time/__asctime.c
index 1853580..fc4d61d 100644
--- a/src/time/__asctime.c
+++ b/src/time/__asctime.c
@@ -1,5 +1,6 @@
 #include <time.h>
 #include <stdio.h>
+#include <signal.h>
 #include <langinfo.h>
 
 const char *__langinfo(nl_item);
@@ -21,7 +22,7 @@ char *__asctime(const struct tm *tm, char *buf)
 		 * application developers that they may not be so lucky
 		 * on other implementations (e.g. stack smashing..).
 		 */
-		*(int*)0 = 0;
+		raise(SIGSEGV);
 	}
 	return buf;
 }
-- 
1.7.5.2


[-- Attachment #5: 0004-Dead-code-elimination.patch --]
[-- Type: application/octet-stream, Size: 2995 bytes --]

From 7e37b1c58d28fc0fb2de5f6b29903bcdba8da250 Mon Sep 17 00:00:00 2001
From: Igmar Palsenberg <igmar@palsenberg.com>
Date: Mon, 6 Jun 2011 16:57:02 +0200
Subject: [PATCH 4/6] Dead code elimination

---
 src/regex/regcomp.c   |    3 +--
 src/regex/regexec.c   |    4 ----
 src/stdio/vfwprintf.c |    3 +--
 3 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/src/regex/regcomp.c b/src/regex/regcomp.c
index 3307942..cb7e272 100644
--- a/src/regex/regcomp.c
+++ b/src/regex/regcomp.c
@@ -1845,13 +1845,12 @@ tre_add_tags(tre_mem_t mem, tre_stack_t *stack, tre_ast_node_t *tree,
 
 	case ADDTAGS_AFTER_ITERATION:
 	  {
-	    int enter_tag;
 	    node = tre_stack_pop(stack);
 	    if (first_pass)
 		node->num_tags = ((tre_iteration_t *)node->obj)->arg->num_tags
 		  + (int)tre_stack_pop(stack);
 	    else
-		enter_tag = (int)tre_stack_pop(stack);
+		(int)tre_stack_pop(stack);
 
 	    DPRINT(("After iteration\n"));
 	    direction = TRE_TAG_MAXIMIZE;
diff --git a/src/regex/regexec.c b/src/regex/regexec.c
index f7aef50..5aa52d9 100644
--- a/src/regex/regexec.c
+++ b/src/regex/regexec.c
@@ -180,7 +180,6 @@ tre_tnfa_run_parallel(const tre_tnfa_t *tnfa, const void *string, int len,
   int num_tags, i;
 
   int match_eo = -1;	   /* end offset of match (-1 if no match found yet) */
-  int new_match = 0;
   int *tmp_tags = NULL;
   int *tmp_iptr;
 
@@ -287,7 +286,6 @@ tre_tnfa_run_parallel(const tre_tnfa_t *tnfa, const void *string, int len,
 		    {
 		      DPRINT(("	 found empty match\n"));
 		      match_eo = pos;
-		      new_match = 1;
 		      for (i = 0; i < num_tags; i++)
 			match_tags[i] = reach_next_i->tags[i];
 		    }
@@ -387,7 +385,6 @@ tre_tnfa_run_parallel(const tre_tnfa_t *tnfa, const void *string, int len,
 			{
 			  DPRINT(("  found match %p\n", trans_i->state));
 			  match_eo = pos;
-			  new_match = 1;
 			  for (i = 0; i < num_tags; i++)
 			    match_tags[i] = reach_next_i->tags[i];
 			}
@@ -411,7 +408,6 @@ tre_tnfa_run_parallel(const tre_tnfa_t *tnfa, const void *string, int len,
 			    {
 			      DPRINT(("	 found better match\n"));
 			      match_eo = pos;
-			      new_match = 1;
 			      for (i = 0; i < num_tags; i++)
 				match_tags[i] = tmp_tags[i];
 			    }
diff --git a/src/stdio/vfwprintf.c b/src/stdio/vfwprintf.c
index 42ce304..064adb9 100644
--- a/src/stdio/vfwprintf.c
+++ b/src/stdio/vfwprintf.c
@@ -160,7 +160,7 @@ static const char sizeprefix['y'-'a'] = {
 
 static int wprintf_core(FILE *f, const wchar_t *fmt, va_list *ap, union arg *nl_arg, int *nl_type)
 {
-	wchar_t *a, *z, *s=(wchar_t *)fmt, *s0;
+	wchar_t *a, *z, *s=(wchar_t *)fmt;
 	unsigned l10n=0, litpct, fl;
 	int w, p;
 	union arg arg;
@@ -235,7 +235,6 @@ static int wprintf_core(FILE *f, const wchar_t *fmt, va_list *ap, union arg *nl_
 		} else p = -1;
 
 		/* Format specifier state machine */
-		s0=s;
 		st=0;
 		do {
 			if (OOB(*s)) return -1;
-- 
1.7.5.2


[-- Attachment #6: 0005-Use-proper-cast-to-wchar_t.patch --]
[-- Type: application/octet-stream, Size: 837 bytes --]

From 9ae03979b02f7ba9f0016ebb2ec3f31094f48fa0 Mon Sep 17 00:00:00 2001
From: Igmar Palsenberg <igmar@palsenberg.com>
Date: Mon, 6 Jun 2011 17:01:44 +0200
Subject: [PATCH 5/6] Use proper cast to wchar_t *

---
 src/stdio/vfwprintf.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/stdio/vfwprintf.c b/src/stdio/vfwprintf.c
index 064adb9..6e7bb5d 100644
--- a/src/stdio/vfwprintf.c
+++ b/src/stdio/vfwprintf.c
@@ -185,7 +185,7 @@ static int wprintf_core(FILE *f, const wchar_t *fmt, va_list *ap, union arg *nl_
 
 		/* Handle literal text and %% format specifiers */
 		for (a=s; *s && *s!='%'; s++);
-		litpct = wcsspn(s, L"%")/2; /* Optimize %%%% runs */
+		litpct = wcsspn(s, (wchar_t *)L"%")/2; /* Optimize %%%% runs */
 		z = s+litpct;
 		s += 2*litpct;
 		l = z-a;
-- 
1.7.5.2


[-- Attachment #7: 0006-weak-alias-fix.patch --]
[-- Type: application/octet-stream, Size: 1977 bytes --]

From f98e49f1c9cc282161f19ec517dd16edc20170fc Mon Sep 17 00:00:00 2001
From: Igmar Palsenberg <igmar@palsenberg.com>
Date: Mon, 6 Jun 2011 17:02:30 +0200
Subject: [PATCH 6/6] You can't weak alias a static function or variable

---
 src/thread/cancel_dummy.c   |    3 ++-
 src/thread/pthread_create.c |    2 +-
 src/thread/pthread_self.c   |    2 +-
 src/time/timer_create.c     |    2 +-
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/thread/cancel_dummy.c b/src/thread/cancel_dummy.c
index a39117e..dac340d 100644
--- a/src/thread/cancel_dummy.c
+++ b/src/thread/cancel_dummy.c
@@ -1,6 +1,7 @@
 #include "pthread_impl.h"
+#include "syscall.h"
 
-static long sccp(long nr, long u, long v, long w, long x, long y, long z)
+long sccp(long nr, long u, long v, long w, long x, long y, long z)
 {
 	return (__syscall)(nr, u, v, w, x, y, z);
 }
diff --git a/src/thread/pthread_create.c b/src/thread/pthread_create.c
index a645f9f..bb13e8f 100644
--- a/src/thread/pthread_create.c
+++ b/src/thread/pthread_create.c
@@ -1,6 +1,6 @@
 #include "pthread_impl.h"
 
-static void dummy_0()
+void dummy_0()
 {
 }
 weak_alias(dummy_0, __rsyscall_lock);
diff --git a/src/thread/pthread_self.c b/src/thread/pthread_self.c
index 55d20c9..4ec5e5c 100644
--- a/src/thread/pthread_self.c
+++ b/src/thread/pthread_self.c
@@ -3,7 +3,7 @@
 static struct pthread main_thread;
 
 /* pthread_key_create.c overrides this */
-static const void *dummy[1] = { 0 };
+const void *dummy[1] = { 0 };
 weak_alias(dummy, __pthread_tsd_main);
 
 static int *errno_location()
diff --git a/src/time/timer_create.c b/src/time/timer_create.c
index 1561d79..795fab8 100644
--- a/src/time/timer_create.c
+++ b/src/time/timer_create.c
@@ -13,7 +13,7 @@ struct start_args {
 	struct sigevent *sev;
 };
 
-static void dummy_1(pthread_t self)
+void dummy_1(pthread_t self)
 {
 }
 weak_alias(dummy_1, __pthread_tsd_run_dtors);
-- 
1.7.5.2

Re: Cleanup patches

[Szabolcs Nagy]

* Igmar Palsenberg <igmar@palsenberg.com> [2011-06-06 17:40:35 +0200]:
> See attached patches for a clang cleanup. No functional changes, just making sure it compiles without warnings.
> 

--- a/Makefile
+++ b/Makefile
@@ -63,7 +63,7 @@ include/bits/alltypes.h: include/bits/alltypes.h.sh
 	sh $< > $@
 
 %.o: $(ARCH)/%.s
-	$(CC) $(CFLAGS) $(INC) -c -o $@ $<
+	$(CC) -c -o $@ $<

this could be $(AS) -o $@ $<

--- a/src/malloc/malloc.c
+++ b/src/malloc/malloc.c
@@ -16,9 +16,9 @@ void *__mremap(void *, size_t, size_t, int, ...);
 int __madvise(void *, size_t, int);
 
 struct chunk {
-	size_t data[1];
 	struct chunk *next;
 	struct chunk *prev;
+	size_t data[];
 };

this does not seem to be correct
c->data[-1] now means something different


--- a/src/regex/regcomp.c
+++ b/src/regex/regcomp.c
@@ -1845,13 +1845,12 @@ tre_add_tags(tre_mem_t mem, tre_stack_t *stack, tre_ast_node_t *tree,
 
 	case ADDTAGS_AFTER_ITERATION:
 	  {
-	    int enter_tag;
 	    node = tre_stack_pop(stack);
 	    if (first_pass)
 		node->num_tags = ((tre_iteration_t *)node->obj)->arg->num_tags
 		  + (int)tre_stack_pop(stack);
 	    else
-		enter_tag = (int)tre_stack_pop(stack);
+		(int)tre_stack_pop(stack);

the (int) cast can go as well..

--- a/src/stdio/vfwprintf.c
+++ b/src/stdio/vfwprintf.c
@@ -185,7 +185,7 @@ static int wprintf_core(FILE *f, const wchar_t *fmt, va_list *ap, union arg *nl_
 
 		/* Handle literal text and %% format specifiers */
 		for (a=s; *s && *s!='%'; s++);
-		litpct = wcsspn(s, L"%")/2; /* Optimize %%%% runs */
+		litpct = wcsspn(s, (wchar_t *)L"%")/2; /* Optimize %%%% runs */
 		z = s+litpct;
 		s += 2*litpct;
 		l = z-a;

this seems wrong
L"" is already a wchar_t string literal
and wcsspn arguments must be const qualified


Subject: [PATCH 6/6] You can't weak alias a static function or variable

you can, at least gcc/ld allows it, it just does not make much sense
but the solution is bad, polluting the public namespace is not ok

Re: Cleanup patches

[Rich Felker]

On Mon, Jun 06, 2011 at 07:13:18PM +0200, Szabolcs Nagy wrote:
>  %.o: $(ARCH)/%.s
> -	$(CC) $(CFLAGS) $(INC) -c -o $@ $<
> +	$(CC) -c -o $@ $<
> 
> this could be $(AS) -o $@ $<

Is there a reason this is necessary or beneficial?

>  struct chunk {
> -	size_t data[1];
>  	struct chunk *next;
>  	struct chunk *prev;
> +	size_t data[];
>  };
> 
> this does not seem to be correct
> c->data[-1] now means something different

This is definitely wrong. data[1] is not a flexible array member. If
this is causing serious problems I could update everything to use
structure pointers offset back by 1 word, and then use size_t
prev_size, size; or similar. But if I remember correctly that was
making the code a good big uglier when I first tried it.

> -	    int enter_tag;
>  	    node = tre_stack_pop(stack);
>  	    if (first_pass)
>  		node->num_tags = ((tre_iteration_t *)node->obj)->arg->num_tags
>  		  + (int)tre_stack_pop(stack);
>  	    else
> -		enter_tag = (int)tre_stack_pop(stack);
> +		(int)tre_stack_pop(stack);
> 
> the (int) cast can go as well..

Indeed. Short of bugs though my intent is not to touch the TRE code,
since I plan to replace it anyway.

>  		/* Handle literal text and %% format specifiers */
>  		for (a=s; *s && *s!='%'; s++);
> -		litpct = wcsspn(s, L"%")/2; /* Optimize %%%% runs */
> +		litpct = wcsspn(s, (wchar_t *)L"%")/2; /* Optimize %%%% runs */
>  		z = s+litpct;
>  		s += 2*litpct;
>  		l = z-a;
> 
> this seems wrong
> L"" is already a wchar_t string literal

I'm guessing this might be an issue of some 32-bit x86 compilers
disagreeing on whether wchar_t is "int" or "long". Traditionally it
was "long" which worked but was obviously stupid conceptually. I don't
know a good way to make musl's wchar.h adapt to what the compiler
wants though...

> and wcsspn arguments must be const qualified

This is wrong. A non-const-qualified pointer always implicitly
converts to the const-qualified version.

> Subject: [PATCH 6/6] You can't weak alias a static function or variable
> 
> you can, at least gcc/ld allows it, it just does not make much sense

It does make sense to allow it, but I can see how it might be a little
more work for the compiler and the compiler might not want to support
it.

> but the solution is bad, polluting the public namespace is not ok

Indeed, the names will all need changing if this is necessary.

Rich

Re: Cleanup patches

[Szabolcs Nagy]

* Rich Felker <dalias@aerifal.cx> [2011-06-06 13:32:10 -0400]:
> On Mon, Jun 06, 2011 at 07:13:18PM +0200, Szabolcs Nagy wrote:
> >  %.o: $(ARCH)/%.s
> > -	$(CC) $(CFLAGS) $(INC) -c -o $@ $<
> > +	$(CC) -c -o $@ $<
> > 
> > this could be $(AS) -o $@ $<
> 
> Is there a reason this is necessary or beneficial?
> 
the c compiler may use different asm syntax than you do?
so $(AS) can be something that knows your .s syntax

> > and wcsspn arguments must be const qualified
> 
> This is wrong. A non-const-qualified pointer always implicitly
> converts to the const-qualified version.
> 
true

> > Subject: [PATCH 6/6] You can't weak alias a static function or variable
> > 
> > you can, at least gcc/ld allows it, it just does not make much sense
> 
> It does make sense to allow it, but I can see how it might be a little
> more work for the compiler and the compiler might not want to support
> it.
> 
what's the difference between
 static int f() { stuff(); }
 int g() __attribute__((weak, alias("f")));
and
 int g() { stuff(); }
?

in the later case local f calls must be replaced by g calls
but other than that is there a difference?

Re: Cleanup patches

[Rich Felker]

On Mon, Jun 06, 2011 at 08:31:15PM +0200, Szabolcs Nagy wrote:
> the c compiler may use different asm syntax than you do?
> so $(AS) can be something that knows your .s syntax

Except that inline asm is also used, so they must agree.

> what's the difference between
>  static int f() { stuff(); }
>  int g() __attribute__((weak, alias("f")));
> and
>  int g() { stuff(); }

The former provides a tentative definition of g which will avoid
pulling in other .o files from an archive (.a) to satisfy references
to g, but which allows another .o file which defines g to override the
tentative definition.

The latter provides a full definition of g which will result in link
errors if g is defined elsewhere.

Rich

Re: Cleanup patches

[Rich Felker]

On Mon, Jun 06, 2011 at 05:40:35PM +0200, Igmar Palsenberg wrote:
> diff --git a/include/dirent.h b/include/dirent.h
> index ca000bd..5496be3 100644
> --- a/include/dirent.h
> +++ b/include/dirent.h
> @@ -18,7 +18,7 @@ struct dirent
>  	off_t d_off;
>  	unsigned short d_reclen;
>  	unsigned char d_type;
> -	char d_name[1];
> +	char d_name[];
>  };

Fixed in a different way that also gives better glibc
abi-compatibility.

> +++ b/src/malloc/malloc.c
> @@ -16,9 +16,9 @@ void *__mremap(void *, size_t, size_t, int, ...);
>  int __madvise(void *, size_t, int);
>  
>  struct chunk {
> -	size_t data[1];
>  	struct chunk *next;
>  	struct chunk *prev;
> +	size_t data[];
>  };

This is plain wrong but if needed I can adjust the code to avoid array
bounds issues.

Rich

Re: Cleanup patches

[Rich Felker]

On Mon, Jun 06, 2011 at 05:40:35PM +0200, Igmar Palsenberg wrote:
> -		if ((uintptr_t)base < mal.brk) *(char *)0=0;
> +		if ((uintptr_t)base < mal.brk)
> +			raise(SIGSEGV);
[etc.]

Fixed using (volatile char *)0=0; instead. As per the original commit
message for this code, the intent is to provide some added safety and
debugging assistance without any significant increase in code size or
bloat.

Rich

Re: Cleanup patches

[Igmar Palsenberg]

> On Mon, Jun 06, 2011 at 07:13:18PM +0200, Szabolcs Nagy wrote:
>> %.o: $(ARCH)/%.s
>> -	$(CC) $(CFLAGS) $(INC) -c -o $@ $<
>> +	$(CC) -c -o $@ $<
>> 
>> this could be $(AS) -o $@ $<
> 
> Is there a reason this is necessary or beneficial?

Clang complains about them. You could ignore them if you want : Werror doesn't seem to be affected by this. It might use an internal assembler, I'm not that familiar with clang internals. Looking at the flags, they don't seem to matter when it comes to assembling .s files.

>> struct chunk {
>> -	size_t data[1];
>> 	struct chunk *next;
>> 	struct chunk *prev;
>> +	size_t data[];
>> };
>> 
>> this does not seem to be correct
>> c->data[-1] now means something different
> 
> This is definitely wrong. data[1] is not a flexible array member. If
> this is causing serious problems I could update everything to use
> structure pointers offset back by 1 word, and then use size_t
> prev_size, size; or similar. But if I remember correctly that was
> making the code a good big uglier when I first tried it.

I need to check. I get out of bounds warning with this code. I'll check and update this code.


>> -	    int enter_tag;
>> 	    node = tre_stack_pop(stack);
>> 	    if (first_pass)
>> 		node->num_tags = ((tre_iteration_t *)node->obj)->arg->num_tags
>> 		  + (int)tre_stack_pop(stack);
>> 	    else
>> -		enter_tag = (int)tre_stack_pop(stack);
>> +		(int)tre_stack_pop(stack);
>> 
>> the (int) cast can go as well..
> 
> Indeed. Short of bugs though my intent is not to touch the TRE code,
> since I plan to replace it anyway.
> 
>> 		/* Handle literal text and %% format specifiers */
>> 		for (a=s; *s && *s!='%'; s++);
>> -		litpct = wcsspn(s, L"%")/2; /* Optimize %%%% runs */
>> +		litpct = wcsspn(s, (wchar_t *)L"%")/2; /* Optimize %%%% runs */
>> 		z = s+litpct;
>> 		s += 2*litpct;
>> 		l = z-a;
>> 
>> this seems wrong
>> L"" is already a wchar_t string literal
> 
> I'm guessing this might be an issue of some 32-bit x86 compilers
> disagreeing on whether wchar_t is "int" or "long". Traditionally it
> was "long" which worked but was obviously stupid conceptually. I don't
> know a good way to make musl's wchar.h adapt to what the compiler
> wants though...

The cast should be OK. In cases where it is correct (and the cast isn't necessary), it is simply a NOOP.

>> and wcsspn arguments must be const qualified
> 
> This is wrong. A non-const-qualified pointer always implicitly
> converts to the const-qualified version.

Indeed.

>> Subject: [PATCH 6/6] You can't weak alias a static function or variable
>> 
>> you can, at least gcc/ld allows it, it just does not make much sense
> 
> It does make sense to allow it, but I can see how it might be a little
> more work for the compiler and the compiler might not want to support
> it.
> 
>> but the solution is bad, polluting the public namespace is not ok
> 
> Indeed, the names will all need changing if this is necessary.

There is a clang bug somewhere never the less : I get weird errors with the macro, if I expand them directly in the code, I get a different error. It should give the same error in both cases. I'll recheck this. We might want to make those function hidden if that solves the problem.



	Igmar

Re: Cleanup patches

[Szabolcs Nagy]

* Igmar Palsenberg <musl@palsenberg.com> [2011-06-07 10:44:44 +0200]:
> The cast should be OK. In cases where it is correct (and the cast isn't necessary), it is simply a NOOP.
> 

a superfluous type cast is not OK even if it's a NOOP
it's ugly, dangerous and surprising

in this case the cast hides an important warning:
wchar_t of musl and the compiler is different
the solution is to define wchar_t properly

it seems pcc and gcc defines __WCHAR_TYPE__
if clang has it defined as well then i guess musl can use it
and fall back to int when it's not defined

Re: Cleanup patches

[Igmar Palsenberg]

> a superfluous type cast is not OK even if it's a NOOP
> it's ugly, dangerous and surprising
> 
> in this case the cast hides an important warning:
> wchar_t of musl and the compiler is different
> the solution is to define wchar_t properly
> 
> it seems pcc and gcc defines __WCHAR_TYPE__
> if clang has it defined as well then i guess musl can use it
> and fall back to int when it's not defined

Indeed.. I was bluntly assuming all compilers use the same type. I'll see if I can fix it the proper way.



	Igmar

Re: Cleanup patches

[Igmar Palsenberg]

> --- a/Makefile
> +++ b/Makefile
> @@ -63,7 +63,7 @@ include/bits/alltypes.h: include/bits/alltypes.h.sh
> 	sh $< > $@
> 
> %.o: $(ARCH)/%.s
> -	$(CC) $(CFLAGS) $(INC) -c -o $@ $<
> +	$(CC) -c -o $@ $<
> 
> this could be $(AS) -o $@ $<

No, because it then assume the .s file has an entrypoint.



	Igmar

Re: Cleanup patches

[Rich Felker]

On Tue, Jun 07, 2011 at 10:44:44AM +0200, Igmar Palsenberg wrote:
> >> this could be $(AS) -o $@ $<
> > 
> > Is there a reason this is necessary or beneficial?
> 
> Clang complains about them. You could ignore them if you want :
> Werror doesn't seem to be affected by this. It might use an internal
> assembler, I'm not that familiar with clang internals. Looking at
> the flags, they don't seem to matter when it comes to assembling .s
> files.

CFLAGS is there for the person compiling musl to be able to add flags.
If it were just a fixed set of flags there'd be no need for a
variable.. Thus I'm a bit hesitant to omit them without further
consideration.

> I need to check. I get out of bounds warning with this code. I'll
> check and update this code.

Yes, you'll get array bounds warnings. You can leave this warning off
unless clang is really miscompiling the code, in which case I'll have
to make some larger changes...

> > I'm guessing this might be an issue of some 32-bit x86 compilers
> > disagreeing on whether wchar_t is "int" or "long". Traditionally it
> > was "long" which worked but was obviously stupid conceptually. I don't
> > know a good way to make musl's wchar.h adapt to what the compiler
> > wants though...
> 
> The cast should be OK. In cases where it is correct (and the cast
> isn't necessary), it is simply a NOOP.

No, the cast, like ALMOST ALL CASTS, hides a bug: that wchar_t is
defined in an inconsistent way. It's actively harmful.

> > Indeed, the names will all need changing if this is necessary.
> 
> There is a clang bug somewhere never the less : I get weird errors
> with the macro, if I expand them directly in the code, I get a
> different error. It should give the same error in both cases. I'll
> recheck this. We might want to make those function hidden if that
> solves the problem.

Don't know what you mean by expanding them directly in the code. You
**cannot** replace the calls to the alias with calls to the dummy
function. If you could, why would I have ever created the weak aliases
to begin with?? The weak semantics are essential to the code working.

Rich

Re: Cleanup patches

[Igmar Palsenberg]

On Jun 7, 2011, at 4:51 PM, Rich Felker wrote:

> On Tue, Jun 07, 2011 at 10:44:44AM +0200, Igmar Palsenberg wrote:
>>>> this could be $(AS) -o $@ $<
>>> 
>>> Is there a reason this is necessary or beneficial?
>> 
>> Clang complains about them. You could ignore them if you want :
>> Werror doesn't seem to be affected by this. It might use an internal
>> assembler, I'm not that familiar with clang internals. Looking at
>> the flags, they don't seem to matter when it comes to assembling .s
>> files.
> 
> CFLAGS is there for the person compiling musl to be able to add flags.
> If it were just a fixed set of flags there'd be no need for a
> variable.. Thus I'm a bit hesitant to omit them without further
> consideration.

The assembler doesn't see those flags. Those flags are simply ignore, but clang does complain about them.

>> I need to check. I get out of bounds warning with this code. I'll
>> check and update this code.
> 
> Yes, you'll get array bounds warnings. You can leave this warning off
> unless clang is really miscompiling the code, in which case I'll have
> to make some larger changes...

Probably not. I might run a case through valgrind to see if it comes up (and if possible, since the last time I checked valgrind, it couldn't handle static binaries).

>>> I'm guessing this might be an issue of some 32-bit x86 compilers
>>> disagreeing on whether wchar_t is "int" or "long". Traditionally it
>>> was "long" which worked but was obviously stupid conceptually. I don't
>>> know a good way to make musl's wchar.h adapt to what the compiler
>>> wants though...
>> 
>> The cast should be OK. In cases where it is correct (and the cast
>> isn't necessary), it is simply a NOOP.
> 
> No, the cast, like ALMOST ALL CASTS, hides a bug: that wchar_t is
> defined in an inconsistent way. It's actively harmful.

I'll make a proper fix for that.

>>> Indeed, the names will all need changing if this is necessary.
>> 
>> There is a clang bug somewhere never the less : I get weird errors
>> with the macro, if I expand them directly in the code, I get a
>> different error. It should give the same error in both cases. I'll
>> recheck this. We might want to make those function hidden if that
>> solves the problem.
> 
> Don't know what you mean by expanding them directly in the code. You
> **cannot** replace the calls to the alias with calls to the dummy
> function. If you could, why would I have ever created the weak aliases
> to begin with?? The weak semantics are essential to the code working.

The code is normally something like :

weak_alias(real, alias);
where real is a static function.

clang then barked about the function being unused. If I expand the macro (gcc -E), and replace weak_alias(...) with the expanded form, I get an error that an alias won't work on a static function.
I need to dive into this to see who to blame :)


	Igmar

Re: Cleanup patches

[Rich Felker]

On Tue, Jun 07, 2011 at 05:26:22PM +0200, Igmar Palsenberg wrote:
> The assembler doesn't see those flags. Those flags are simply
> ignore, but clang does complain about them.

What about things like -m32? Surely that affects the assembler.

> >> I need to check. I get out of bounds warning with this code. I'll
> >> check and update this code.
> > 
> > Yes, you'll get array bounds warnings. You can leave this warning off
> > unless clang is really miscompiling the code, in which case I'll have
> > to make some larger changes...
> 
> Probably not. I might run a case through valgrind to see if it comes up (and if possible, since the last time I checked valgrind, it couldn't handle static binaries).

Just test malloc and make sure it works. If clang generated wrong
code, it will fail catastrophically.

> >>> I'm guessing this might be an issue of some 32-bit x86 compilers
> >>> disagreeing on whether wchar_t is "int" or "long". Traditionally it
> >>> was "long" which worked but was obviously stupid conceptually. I don't
> >>> know a good way to make musl's wchar.h adapt to what the compiler
> >>> wants though...
> >> 
> >> The cast should be OK. In cases where it is correct (and the cast
> >> isn't necessary), it is simply a NOOP.
> > 
> > No, the cast, like ALMOST ALL CASTS, hides a bug: that wchar_t is
> > defined in an inconsistent way. It's actively harmful.
> 
> I'll make a proper fix for that.

I think it should be fixed in git. Please tell me if it's not.

> The code is normally something like :
> 
> weak_alias(real, alias);
> where real is a static function.
> 
> clang then barked about the function being unused. If I expand the
> macro (gcc -E), and replace weak_alias(...) with the expanded form,
> I get an error that an alias won't work on a static function.

Are you sure you didn't botch the macro expansion? If you did it right
and the warnings/errors differ, that's a bug in clang, I think... Can
you show an example of the expansion you did? (just the relevant lines)

> I need to dive into this to see who to blame :)

:)

Rich