[PATCH 0/2] Resolve compiler warnings in master

[PATCH 0/2] Resolve compiler warnings in master

[Samuel Holland]

These two patches resolve some compiler warnings about mismatched
attributes and restrict violations. There's another warning, related
to duplicate definitions of TIOCSER_TEMT on some arches; I'm not sure
which header needs to be changed. It results in:

./include/sys/ioctl.h:47: warning: "TIOCSER_TEMT" redefined

The definitions are:

arch/mips/bits/termios.h:#define TIOCSER_TEMT 0x01
arch/mips64/bits/termios.h:#define TIOCSER_TEMT 0x01
arch/mipsn32/bits/termios.h:#define TIOCSER_TEMT 0x01
arch/powerpc/bits/termios.h:#define TIOCSER_TEMT 0x01
arch/powerpc64/bits/termios.h:#define TIOCSER_TEMT 0x01
include/sys/ioctl.h:#define TIOCSER_TEMT 1

Samuel Holland (2):
  resolve -Wrestrict warnings
  use the correct attributes for ___errno_location

 src/aio/lio_listio.c         | 6 +++---
 src/errno/__errno_location.c | 6 +++++-
 src/signal/sigset.c          | 8 ++++----
 src/unistd/ualarm.c          | 6 +++---
 4 files changed, 15 insertions(+), 11 deletions(-)

-- 
2.21.0

[PATCH 1/2] resolve -Wrestrict warnings

[Samuel Holland]

The old/new parameters to pthread_sigmask, sigprocmask, and setitimer
are marked __restrict, so passing the same address to both is
prohibited. Modify callers of these functions to use a separate object
for each argument.
---
 src/aio/lio_listio.c | 6 +++---
 src/signal/sigset.c  | 8 ++++----
 src/unistd/ualarm.c  | 6 +++---
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/aio/lio_listio.c b/src/aio/lio_listio.c
index 7b6a03d3..0799c15d 100644
--- a/src/aio/lio_listio.c
+++ b/src/aio/lio_listio.c
@@ -113,7 +113,7 @@ int lio_listio(int mode, struct aiocb *restrict const *restrict cbs, int cnt, st
 
 	if (st) {
 		pthread_attr_t a;
-		sigset_t set;
+		sigset_t set, set_old;
 		pthread_t td;
 
 		if (sev->sigev_notify == SIGEV_THREAD) {
@@ -128,13 +128,13 @@ int lio_listio(int mode, struct aiocb *restrict const *restrict cbs, int cnt, st
 		}
 		pthread_attr_setdetachstate(&a, PTHREAD_CREATE_DETACHED);
 		sigfillset(&set);
-		pthread_sigmask(SIG_BLOCK, &set, &set);
+		pthread_sigmask(SIG_BLOCK, &set, &set_old);
 		if (pthread_create(&td, &a, wait_thread, st)) {
 			free(st);
 			errno = EAGAIN;
 			return -1;
 		}
-		pthread_sigmask(SIG_SETMASK, &set, 0);
+		pthread_sigmask(SIG_SETMASK, &set_old, 0);
 	}
 
 	return 0;
diff --git a/src/signal/sigset.c b/src/signal/sigset.c
index 0d7b4564..f3e8c407 100644
--- a/src/signal/sigset.c
+++ b/src/signal/sigset.c
@@ -3,7 +3,7 @@
 void (*sigset(int sig, void (*handler)(int)))(int)
 {
 	struct sigaction sa, sa_old;
-	sigset_t mask;
+	sigset_t mask, mask_old;
 
 	sigemptyset(&mask);
 	if (sigaddset(&mask, sig) < 0)
@@ -12,7 +12,7 @@ void (*sigset(int sig, void (*handler)(int)))(int)
 	if (handler == SIG_HOLD) {
 		if (sigaction(sig, 0, &sa_old) < 0)
 			return SIG_ERR;
-		if (sigprocmask(SIG_BLOCK, &mask, &mask) < 0)
+		if (sigprocmask(SIG_BLOCK, &mask, &mask_old) < 0)
 			return SIG_ERR;
 	} else {
 		sa.sa_handler = handler;
@@ -20,8 +20,8 @@ void (*sigset(int sig, void (*handler)(int)))(int)
 		sigemptyset(&sa.sa_mask);
 		if (sigaction(sig, &sa, &sa_old) < 0)
 			return SIG_ERR;
-		if (sigprocmask(SIG_UNBLOCK, &mask, &mask) < 0)
+		if (sigprocmask(SIG_UNBLOCK, &mask, &mask_old) < 0)
 			return SIG_ERR;
 	}
-	return sigismember(&mask, sig) ? SIG_HOLD : sa_old.sa_handler;
+	return sigismember(&mask_old, sig) ? SIG_HOLD : sa_old.sa_handler;
 }
diff --git a/src/unistd/ualarm.c b/src/unistd/ualarm.c
index 855504bc..2985855c 100644
--- a/src/unistd/ualarm.c
+++ b/src/unistd/ualarm.c
@@ -7,7 +7,7 @@ unsigned ualarm(unsigned value, unsigned interval)
 	struct itimerval it = {
 		.it_interval.tv_usec = interval,
 		.it_value.tv_usec = value
-	};
-	setitimer(ITIMER_REAL, &it, &it);
-	return it.it_value.tv_sec*1000000 + it.it_value.tv_usec;
+	}, it_old;
+	setitimer(ITIMER_REAL, &it, &it_old);
+	return it_old.it_value.tv_sec*1000000 + it_old.it_value.tv_usec;
 }
-- 
2.21.0

[PATCH 2/2] use the correct attributes for ___errno_location

[Samuel Holland]

In the public header, __errno_location is declared with the "const"
attribute, conditional on __GNUC__. Ensure that its internal alias has
the same attributes.
---
 src/errno/__errno_location.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/errno/__errno_location.c b/src/errno/__errno_location.c
index 7f9d6027..b59919c3 100644
--- a/src/errno/__errno_location.c
+++ b/src/errno/__errno_location.c
@@ -6,4 +6,8 @@ int *__errno_location(void)
 	return &__pthread_self()->errno_val;
 }
 
-weak_alias(__errno_location, ___errno_location);
+weak_alias(__errno_location, ___errno_location)
+#ifdef __GNUC__
+__attribute__((const))
+#endif
+;
-- 
2.21.0

Re: [PATCH 0/2] Resolve compiler warnings in master

[Rich Felker]

On Sat, Jun 29, 2019 at 04:22:42PM -0500, Samuel Holland wrote:
> These two patches resolve some compiler warnings about mismatched
> attributes and restrict violations. There's another warning, related
> to duplicate definitions of TIOCSER_TEMT on some arches; I'm not sure
> which header needs to be changed. It results in:
> 
> ../include/sys/ioctl.h:47: warning: "TIOCSER_TEMT" redefined
> 
> The definitions are:
> 
> arch/mips/bits/termios.h:#define TIOCSER_TEMT 0x01
> arch/mips64/bits/termios.h:#define TIOCSER_TEMT 0x01
> arch/mipsn32/bits/termios.h:#define TIOCSER_TEMT 0x01
> arch/powerpc/bits/termios.h:#define TIOCSER_TEMT 0x01
> arch/powerpc64/bits/termios.h:#define TIOCSER_TEMT 0x01
> include/sys/ioctl.h:#define TIOCSER_TEMT 1

I don't see how this is happening with a consistent tree. Commit
3517d74a5e04a377192d1f4882ad6c8dc22ce69a removed it from the bits
headers.

Rich

Re: [PATCH 2/2] use the correct attributes for ___errno_location

[Rich Felker]

On Sat, Jun 29, 2019 at 04:22:44PM -0500, Samuel Holland wrote:
> In the public header, __errno_location is declared with the "const"
> attribute, conditional on __GNUC__. Ensure that its internal alias has
> the same attributes.
> ---
>  src/errno/__errno_location.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/errno/__errno_location.c b/src/errno/__errno_location.c
> index 7f9d6027..b59919c3 100644
> --- a/src/errno/__errno_location.c
> +++ b/src/errno/__errno_location.c
> @@ -6,4 +6,8 @@ int *__errno_location(void)
>  	return &__pthread_self()->errno_val;
>  }
>  
> -weak_alias(__errno_location, ___errno_location);
> +weak_alias(__errno_location, ___errno_location)
> +#ifdef __GNUC__
> +__attribute__((const))
> +#endif
> +;
> -- 
> 2.21.0

Thanks for catching this. It's probably a significant regression in
codegen. I think the attribute should be on the declaration in
src/include/errno.h though, not on the weak_alias definition. Most
importantly this is needed for it to affect codegen in the callers.
But it's also for consistency with the approach of having attributes
on the declarations rather than the definitions (to ensure everyone
gets a consistent view of them), and to avoid assumptions about what
the weak_alias macro expands to.

Rich

Re: [PATCH 0/2] Resolve compiler warnings in master

[Samuel Holland]

On 6/29/19 4:46 PM, Rich Felker wrote:
> On Sat, Jun 29, 2019 at 04:22:42PM -0500, Samuel Holland wrote:
>> These two patches resolve some compiler warnings about mismatched
>> attributes and restrict violations. There's another warning, related
>> to duplicate definitions of TIOCSER_TEMT on some arches; I'm not sure
>> which header needs to be changed. It results in:
>>
>> ../include/sys/ioctl.h:47: warning: "TIOCSER_TEMT" redefined
>>
>> The definitions are:
>>
>> arch/mips/bits/termios.h:#define TIOCSER_TEMT 0x01
>> arch/mips64/bits/termios.h:#define TIOCSER_TEMT 0x01
>> arch/mipsn32/bits/termios.h:#define TIOCSER_TEMT 0x01
>> arch/powerpc/bits/termios.h:#define TIOCSER_TEMT 0x01
>> arch/powerpc64/bits/termios.h:#define TIOCSER_TEMT 0x01
>> include/sys/ioctl.h:#define TIOCSER_TEMT 1
> 
> I don't see how this is happening with a consistent tree. Commit
> 3517d74a5e04a377192d1f4882ad6c8dc22ce69a removed it from the bits
> headers.

That commit removed it from bits/ioctl.h. The definition is still in
bits/termios.h, where it was changed in 9eda4dc69c33852c97c6f69176bf45ffc80b522f
to match the old value in bits/ioctl.h. So maybe sys/ioctl.h needs to be changed
from 1 to 0x01?

Cheers,
Samuel