[PATCH 0/2] Fixing clang compilation warnings
Hi, now that silent rules are in place it is also easier to spot compilation warnings with clang (CC=clang ./configure && make), so let's fix these as they seem pretty straightforward. The two patches in this series fix two classes of clang warnings, so that dash can now compile cleanly with it too. This also makes the output of "scan-build make" more usable, however fixing the issues found by the static analyzer is not something I am going to work on anytime soon. Ciao, Antonio Antonio Ospite (2): Fix clang warnings about "string plus integer" Fix clang warnings about GNU old-style field designator src/eval.c | 3 ++- src/jobs.c | 3 ++- src/output.c | 12 ++-- 3 files changed, 10 insertions(+), 8 deletions(-) -- Antonio Ospite https://ao2.it https://twitter.com/ao2it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing?
[PATCH 2/2] Fix clang warnings about GNU old-style field designator
Building with clang results in some warnings about the use of GNU
old-style field designators:
---
output.c:86:2: warning: use of GNU old-style field designator extension
[-Wgnu-designator]
nextc: 0, end: 0, buf: 0, bufsize: OUTBUFSIZ, fd: 1, flags: 0
^~
.nextc =
...
---
Fix the issue bu using C99 initializers instead.
This should be safe and should not introduce any compatibility problems
as it is done already in other parts of the codebase, like
src/expand.c:ccmatch() and src/parser.c::readtoken1().
Signed-off-by: Antonio Ospite
---
src/output.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/output.c b/src/output.c
index 34243ea..e9ee9b4 100644
--- a/src/output.c
+++ b/src/output.c
@@ -71,27 +71,27 @@
#ifdef USE_GLIBC_STDIO
struct output output = {
- stream: 0, nextc: 0, end: 0, buf: 0, bufsize: 0, fd: 1, flags: 0
+ .stream = 0, .nextc = 0, .end = 0, .buf = 0, .bufsize = 0, .fd = 1,
.flags = 0
};
struct output errout = {
- stream: 0, nextc: 0, end: 0, buf: 0, bufsize: 0, fd: 2, flags: 0
+ .stream = 0, .nextc = 0, .end = 0, .buf = 0, .bufsize = 0, .fd = 2,
.flags = 0
}
#ifdef notyet
struct output memout = {
- stream: 0, nextc: 0, end: 0, buf: 0, bufsize: 0, fd: MEM_OUT, flags: 0
+ .stream = 0, .nextc = 0, .end = 0, .buf = 0, .bufsize = 0, .fd =
MEM_OUT, .flags = 0
};
#endif
#else
struct output output = {
- nextc: 0, end: 0, buf: 0, bufsize: OUTBUFSIZ, fd: 1, flags: 0
+ .nextc = 0, .end = 0, .buf = 0, .bufsize = OUTBUFSIZ, .fd = 1, .flags = 0
};
struct output errout = {
- nextc: 0, end: 0, buf: 0, bufsize: 0, fd: 2, flags: 0
+ .nextc = 0, .end = 0, .buf = 0, .bufsize = 0, .fd = 2, .flags = 0
};
struct output preverrout;
#ifdef notyet
struct output memout = {
- nextc: 0, end: 0, buf: 0, bufsize: 0, fd: MEM_OUT, flags: 0
+ .nextc = 0, .end = 0, .buf = 0, .bufsize = 0, .fd = MEM_OUT, .flags = 0
};
#endif
#endif
--
2.20.0
Re: [PATCH 0/5] Build system updates and gcc warnings fixes
On Mon, 19 Nov 2018 13:04:45 +0100 Antonio Ospite wrote: > On Sat, 27 Oct 2018 15:35:11 +0200 > Antonio Ospite wrote: > > > On Tue, 16 Oct 2018 18:42:15 +0200 > > Antonio Ospite wrote: > > > > > Hi, > > > > > > here are some build system updates and some fixes for compilation > > > warnings with Gcc. > > > > > > After this patchset, compilation with Gcc is nice and clean, > > > > > [...] > > > .gitignore | 2 ++ > > > configure.ac| 8 +--- > > > src/Makefile.am | 16 > > > src/eval.c | 2 +- > > > src/system.h| 4 > > > 5 files changed, 16 insertions(+), 16 deletions(-) > > > > > > > Looking at the mailing list I see there are some patches sent > > before this series which touch .gitignore and src/Makefile.am > > > > I see that some of those other patches have been merged, and in fact > patch 3/5 in this series can now be skipped. > Hi, can I mark 3/5 as Superseded in patchwork myself? Or would that disrupt your workflow? The intent would be to have one patch less for you to worry about. Thank you, Antonio -- Antonio Ospite https://ao2.it https://twitter.com/ao2it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing?
Re: [PATCH] main: don't raise exception when executing dotcmd() on a non-existent file
On Sat, 24 Nov 2018 18:56:48 +0100 Jilles Tjoelker wrote: > On Sat, Nov 24, 2018 at 05:20:21PM +0100, Antonio Ospite wrote: > > When sourcing a file with the dotcmd() builtin, dash raises an exception > > when the input file cannot be opened. > > > For instance the following script fails prematurely and does not output > > the message: > > > #!/bin/sh > > > . /non-existent-file > > echo "Survived!" > > > In this case, the behavior of dash is different from other shells (e.g. > > bash, zsh) which are more forgiving and allow execution to carry on even > > if the input file cannot be opened. > > POSIX is unambiguous (XCU 2.14 Special Built-In Utilities -> dot) that a > non-interactive shell shall abort when a dot script is not found, and > bash and zsh comply to this when standards compliance is requested (e.g. > by naming the shell "sh" in argv[0] or using "set -o posix" in bash or > "set -o posixbuiltins" in zsh). Most other shells (e.g. mksh, FreeBSD > sh, yash) comply to POSIX unconditionally here, like dash currently > does. > OK, thanks for clarifying that. I guess I was a little lazy for not checking before posting. > > Fix this by passing the INPUT_NOFILE_OK flag when calling setinputfile() > > in dotcmd(). > > > As a bonus, this also fixes cases like the following: > > > set -e > > . /non-existent-file || true > > echo "Survived! Let's do something else..." > > > This idiom is sometimes used in shell script to source a config file > > with default values only to provide fallback values if the default > > values were not available. > > The above code is specific to bash and zsh non-POSIX modes, and will not > work in a #!/bin/sh script unless specific steps are taken (such as "set > +o posix" or starting the script with "bash script1" rather than > "./script1"). > > The simple solution is > [ ! -f /non-existent-file ] || . /non-existent-file > Time-of-check-time-of-use issues should not be an issue for config > files. > Indeed this is the most used form, thank you again. Now I know that the other one is non-standard. Sorry for the noise, Antonio > Alternatively, one could try > command . ./non-existent-file || true > but this may ignore more errors than desired (such as syntax errors in > the sourced file) and does not work correctly in yash 2.30. > > -- > Jilles Tjoelker -- Antonio Ospite https://ao2.it https://twitter.com/ao2it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing?
Re: [v2 PATCH] system: Disable glibc warning on sigsetmask
On Wed, 21 Nov 2018 11:45:04 +0800
Herbert Xu wrote:
> On Tue, Nov 20, 2018 at 11:48:20PM +0100, Antonio Ospite wrote:
> >
> > Just for the record, compiling with clang (CC=clang ./configure && make)
> > still gives a warning:
>
> OK, we could disable it completely unless it's gcc:
>
That works with clang.
> ---8<---
> As sigsetmask is set as deprecated in glibc this patch adds the
> pragmas to disable the warning in gcc around our one and only use
> of sigsetmask.
>
> It also disables it completely for non-gcc compilers and older
> gcc compilers as they may generate a warning too.
>
The "it" in the last sentence may look like it refers to the
warning while AFAIU it refers to the function call itself.
So maybe say "It also disables sigsetmask() completely ..."?
Also a brief note in the commit message about _why_ sigsetmask() is
kept around when using gcc would be useful IMHO.
Thank you,
Antonio
> Reported-by: Antonio Ospite
> Signed-off-by: Herbert Xu
>
> diff --git a/src/system.h b/src/system.h
> index a8d09b3..007952c 100644
> --- a/src/system.h
> +++ b/src/system.h
> @@ -36,8 +36,17 @@
>
> static inline void sigclearmask(void)
> {
> -#ifdef HAVE_SIGSETMASK
> +#if defined(HAVE_SIGSETMASK) && \
> +(!defined(__GLIBC__) || \
> + (defined(__GNUC__) && (__GNUC__ * 1000 + __GNUC_MINOR__) >= 4006))
> +#ifdef __GLIBC__
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> +#endif
> sigsetmask(0);
> +#ifdef __GLIBC__
> +#pragma GCC diagnostic pop
> +#endif
> #else
> sigset_t set;
> sigemptyset();
> --
> Email: Herbert Xu
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Re: eval: Use sh_warnx instead of warnx
On Tue, 20 Nov 2018 10:09:26 +0800
Herbert Xu wrote:
> Antonio Ospite wrote:
> >
> > BTW a new warning was introduced by commit 8e43729 (eval: Report I/O
> > error on stdout, 2018-09-07):
> >
> > CC eval.o
> > eval.c: In function ‘evalbltin’:
> > eval.c:956:3: warning: implicit declaration of function ‘warnx’; did you
> > mean ‘sh_warnx’? [-Wimplicit-function-declaration]
> > warnx("%s: I/O error", commandname);
> > ^
> > sh_warnx
>
> Thanks for the heads up.
>
New warnings stand out more with silent rules on. :)
The change looks good to me.
Thank you,
Antonio
> ---8<---
> This patch fixes a typo in evalbltin where warnx was used instead
> of sh_warnx.
>
> Reported-by: Antonio Ospite
> Fixes: 8e43729547b5 ("eval: Report I/O error on stdout")
>
> diff --git a/src/eval.c b/src/eval.c
> index 546ee1b..c27bc35 100644
> --- a/src/eval.c
> +++ b/src/eval.c
> @@ -953,7 +953,7 @@ evalbltin(const struct builtincmd *cmd, int argc, char
> **argv, int flags)
> status = (*cmd->builtin)(argc, argv);
> flushall();
> if (outerr(out1))
> - warnx("%s: I/O error", commandname);
> + sh_warnx("%s: I/O error", commandname);
> status |= outerr(out1);
> exitstatus = status;
> cmddone:
> --
> Email: Herbert Xu
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Re: system: Disable gcc warning on sigsetmask
On Mon, 19 Nov 2018 18:47:11 +0800
Herbert Xu wrote:
[...]
> I agree with getting rid of the warning, and this seems to work
> for me. Please let me know if you still a get warning on some
> other platform. Thanks!
>
Just for the record, compiling with clang (CC=clang ./configure && make)
still gives a warning:
---
CC system.o
In file included from system.c:62:
./system.h:44:2: warning: 'sigsetmask' is deprecated [-Wdeprecated-declarations]
sigsetmask(0);
^
/usr/include/signal.h:173:44: note: 'sigsetmask' has been explicitly marked
deprecated here
extern int sigsetmask (int __mask) __THROW __attribute_deprecated__;
^
/usr/include/x86_64-linux-gnu/sys/cdefs.h:246:51: note: expanded from macro
'__attribute_deprecated__'
# define __attribute_deprecated__ __attribute__ ((__deprecated__))
^
1 warning generated.
---
This is expected as the patch below only addresses gcc.
If there is a valid reason to keep sigsetmask() around I think we can
live with some warnings in unusual setups, but if the gain is negligible
we might as well just get rid of it.
Thank you,
Antonio
> ---8<---
> As sigsetmask is set as deprecated in glibc this patch adds the
> pragmas to disable the warning in gcc around our one and only use
> of sigsetmask.
>
> Reported-by: Antonio Ospite
> Signed-off-by: Herbert Xu
>
> diff --git a/src/system.h b/src/system.h
> index a8d09b3..f3aa930 100644
> --- a/src/system.h
> +++ b/src/system.h
> @@ -37,7 +37,14 @@
> static inline void sigclearmask(void)
> {
> #ifdef HAVE_SIGSETMASK
> +#if defined(__GNUC__) && (__GNUC__ * 1000 + __GNUC_MINOR__) >= 4006
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
> +#endif
> sigsetmask(0);
> +#if defined(__GNUC__) && (__GNUC__ * 1000 + __GNUC_MINOR__) >= 4006
> +#pragma GCC diagnostic pop
> +#endif
> #else
> sigset_t set;
> sigemptyset();
> --
> Email: Herbert Xu
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Re: [PATCH 0/5] Build system updates and gcc warnings fixes
On Sat, 27 Oct 2018 15:35:11 +0200
Antonio Ospite wrote:
> On Tue, 16 Oct 2018 18:42:15 +0200
> Antonio Ospite wrote:
>
> > Hi,
> >
> > here are some build system updates and some fixes for compilation
> > warnings with Gcc.
> >
> > After this patchset, compilation with Gcc is nice and clean,
> >
> [...]
> > .gitignore | 2 ++
> > configure.ac| 8 +---
> > src/Makefile.am | 16
> > src/eval.c | 2 +-
> > src/system.h| 4
> > 5 files changed, 16 insertions(+), 16 deletions(-)
> >
>
> Looking at the mailing list I see there are some patches sent
> before this series which touch .gitignore and src/Makefile.am
>
I see that some of those other patches have been merged, and in fact
patch 3/5 in this series can now be skipped.
BTW a new warning was introduced by commit 8e43729 (eval: Report I/O
error on stdout, 2018-09-07):
CC eval.o
eval.c: In function ‘evalbltin’:
eval.c:956:3: warning: implicit declaration of function ‘warnx’; did you mean
‘sh_warnx’? [-Wimplicit-function-declaration]
warnx("%s: I/O error", commandname);
^
sh_warnx
> If you apply those first, let me know if you want me to rebase and
> resend this series to minimize conflicts.
>
Ciao,
Antonio
--
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
Re: [PATCH 0/5] Build system updates and gcc warnings fixes
On Tue, 16 Oct 2018 18:42:15 +0200 Antonio Ospite wrote: > Hi, > > here are some build system updates and some fixes for compilation > warnings with Gcc. > > After this patchset, compilation with Gcc is nice and clean, > [...] > .gitignore | 2 ++ > configure.ac| 8 +--- > src/Makefile.am | 16 > src/eval.c | 2 +- > src/system.h| 4 > 5 files changed, 16 insertions(+), 16 deletions(-) > Looking at the mailing list I see there are some patches sent before this series which touch .gitignore and src/Makefile.am If you apply those first, let me know if you want me to rebase and resend this series to minimize conflicts. Thanks you, Antonio -- Antonio Ospite https://ao2.it https://twitter.com/ao2it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing?
[PATCH 2/5] Enable automake silent rules
Enable automake silent rules to make it easier to spot compilation problems. Silent rules will be enabled by default, but only if they are available, in order to keep compatibility with older autotools versions. Prepend the silent strings also to custom rules. Signed-off-by: Antonio Ospite --- configure.ac| 2 ++ src/Makefile.am | 16 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/configure.ac b/configure.ac index 594fb42..036730d 100644 --- a/configure.ac +++ b/configure.ac @@ -4,6 +4,8 @@ AC_CONFIG_SRCDIR([src/main.c]) AC_CONFIG_HEADERS(config.h) +m4_ifdef([AM_SILENT_RULES], [AM_SILENT_RULES(yes)]) + dnl Checks for programs. AC_PROG_CC AC_USE_SYSTEM_EXTENSIONS diff --git a/src/Makefile.am b/src/Makefile.am index 8b9eb8c..1732465 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -44,27 +44,27 @@ EXTRA_DIST = \ mknodes.c nodetypes nodes.c.pat mksyntax.c mksignames.c token.h token_vars.h: mktokens - $(SHELL) $^ + $(AM_V_GEN)$(SHELL) $^ builtins.def: builtins.def.in $(top_builddir)/config.h - $(COMPILE) -E -x c -o $@ $< + $(AM_V_CC)$(COMPILE) -E -x c -o $@ $< builtins.c builtins.h: mkbuiltins builtins.def - $(SHELL) $^ + $(AM_V_GEN)$(SHELL) $^ init.c: mkinit $(dash_CFILES) - ./$^ + $(AM_V_GEN)./$^ nodes.c nodes.h: mknodes nodetypes nodes.c.pat - ./$^ + $(AM_V_GEN)./$^ syntax.c syntax.h: mksyntax - ./$^ + $(AM_V_GEN)./$^ signames.c: mksignames - ./$^ + $(AM_V_GEN)./$^ mksyntax: token.h $(HELPERS): %: %.c - $(COMPILE_FOR_BUILD) -o $@ $< + $(AM_V_CC)$(COMPILE_FOR_BUILD) -o $@ $< -- 2.19.1
[PATCH 4/5] Stop using deprecated function sigsetmask()
sigsetmask() is deprecated, at least on recent glibc; stop using it to
silence the following compiler warning:
---
system.h:40:2: warning: ‘sigsetmask’ is deprecated [-Wdeprecated-declarations]
sigsetmask(0);
^~
In file included from /usr/include/x86_64-linux-gnu/sys/param.h:28,
from shell.h:52,
from nodes.c:46:
/usr/include/signal.h:173:12: note: declared here
extern int sigsetmask (int __mask) __THROW __attribute_deprecated__;
^~
---
Using sigprocmask() and friends unconditionally should not be a problem,
as commit e94a964 (eval: Add vfork support, 2018-05-19) also does it.
Signed-off-by: Antonio Ospite
---
configure.ac | 2 +-
src/system.h | 4
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/configure.ac b/configure.ac
index 036730d..32ef456 100644
--- a/configure.ac
+++ b/configure.ac
@@ -89,7 +89,7 @@ AC_CHECK_DECL([PRIdMAX],,
dnl Checks for library functions.
AC_CHECK_FUNCS(bsearch faccessat getpwnam getrlimit isalpha killpg \
mempcpy \
- sigsetmask stpcpy strchrnul strsignal strtod strtoimax \
+ stpcpy strchrnul strsignal strtod strtoimax \
strtoumax sysconf)
dnl Check whether it's worth working around FreeBSD PR kern/125009.
diff --git a/src/system.h b/src/system.h
index a8d09b3..6950e6e 100644
--- a/src/system.h
+++ b/src/system.h
@@ -36,13 +36,9 @@
static inline void sigclearmask(void)
{
-#ifdef HAVE_SIGSETMASK
- sigsetmask(0);
-#else
sigset_t set;
sigemptyset();
sigprocmask(SIG_SETMASK, , 0);
-#endif
}
#ifndef HAVE_MEMPCPY
--
2.19.1
[PATCH 1/5] Update configure.ac with suggestions from autoupdate
Apply the changes suggested by running autoupdate on the source repository: 1. Properly quote AC_INIT arguments. 2. Use AC_USE_SYSTEM_EXTENSIONS instead of AC_GNU_SOURCE. The former is a superset of the latter, and enables more options, see https://www.gnu.org/software/autoconf/manual/autoconf-2.60/html_node/Posix-Variants.html Signed-off-by: Antonio Ospite --- configure.ac | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/configure.ac b/configure.ac index 4829288..594fb42 100644 --- a/configure.ac +++ b/configure.ac @@ -1,4 +1,4 @@ -AC_INIT(dash, 0.5.10.2) +AC_INIT([dash],[0.5.10.2]) AM_INIT_AUTOMAKE([foreign subdir-objects]) AC_CONFIG_SRCDIR([src/main.c]) @@ -6,7 +6,7 @@ AC_CONFIG_HEADERS(config.h) dnl Checks for programs. AC_PROG_CC -AC_GNU_SOURCE +AC_USE_SYSTEM_EXTENSIONS AC_PROG_YACC AC_MSG_CHECKING([for build system compiler]) -- 2.19.1
Re: [PATCH] eval: make traps work when "set -e" is enabled
On Tue, 16 Oct 2018 14:09:52 +0200 Antonio Ospite wrote: [...] > I am marking the patch as RFC because I don't know the dash codebase very > well, and I might not be aware of possible drawbacks of this change. It worked > in my limited testing but that's it. > I forgot the '--rfc' option when calling git-format-patch, the subject meant to say "[RFC PATCH] ". Ciao, Antonio -- Antonio Ospite https://ao2.it https://twitter.com/ao2it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing?
[PATCH] eval: make traps work when "set -e" is enabled
When "set -e" is enabled traps are not always executed, in particular
the EXIT trap is not executed when the shell exits on an unhandled
error.
Consider the following test script:
#!/bin/dash
set -e
trap 'ret=$?; echo "EXIT: $ret"' EXIT
trap 'exit 2' HUP INT QUIT PIPE TERM
read variable
By pressing Ctrl-C one would expect the EXIT trap to be called, as it is
the case with other shells (bash, zsh), but dash does not do it.
By calling dotrap() before jumping to the exit path when checkexit is
not zero, dash behaves like other shells.
Signed-off-by: Antonio Ospite
---
Hi,
this has been reported in Debian[1] and I noticed the issue myself too, so
I tried to take a look at it.
I am marking the patch as RFC because I don't know the dash codebase very
well, and I might not be aware of possible drawbacks of this change. It worked
in my limited testing but that's it.
I don't know if the behavior of traps is specified when "set -e" is active,
but in case it isn't it would stll be good to behave like other shells.
Any comment is welcome.
Thank you,
Antonio
[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=779416
src/eval.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/eval.c b/src/eval.c
index 6185db4..7d348d0 100644
--- a/src/eval.c
+++ b/src/eval.c
@@ -307,11 +307,11 @@ setstatus:
break;
}
out:
+ dotrap();
+
if (checkexit & status)
goto exexit;
- dotrap();
-
if (flags & EV_EXIT) {
exexit:
exraise(EXEXIT);
--
2.19.1
