build issues when using pcc

build issues when using pcc

[Szabolcs Nagy]

hello i compiled dash with pcc and found two issues:

src/output.h the outc macro is not standard c:
#define outc(c, file)<->((file)->nextc == (file)->end ? outcslow((c), (file)) : (*(file)->nextc = (c), (file)->nextc++))

type signature of the operands are int ? void : char*
but the two sides of : should be compatible
either use
    ... ? outcslow(), 0 : ...
or
    ... ? outcslow() : ..., (void)0
otherwise a standard compliant compiler won't compile it


the other issue is in the mkbuiltins:
it assumes that the cpp does not produce empty lines with
whitespace only
the following line
< $builtins sed '/^#/d; /^$/d' > $temp
should be
< $builtins sed '/^#/d; /^ *$/d' > $temp

so empty lines with spaces are dropped as well
(allowed by the standard after the preprocessing pass)


(also it would be nice to include a configure script in
the git repo, or don't use autoconf in the first place)

Re: build issues when using pcc

[Jonathan Nieder]

Hi,

Szabolcs Nagy wrote:

> hello i compiled dash with pcc and found two issues:
>
> src/output.h the outc macro is not standard c:

Thanks.  I tried and succeeded in reproducing this first one by
noticing that "gcc -pedantic" produces a warning.  It also produces
many other warnings --- for example, one about use of statement
expressions in src/error.h.  It might be nice to fix more of these.

> the following line
> < $builtins sed '/^#/d; /^$/d' > $temp
> should be
> < $builtins sed '/^#/d; /^ *$/d' > $temp
>
> so empty lines with spaces are dropped as well
> (allowed by the standard after the preprocessing pass)

What cpp do you use (so others can catch similar problems in the
future)?
 
Looking over the C standard, it seems that if cc -E were exactly
carrying out translation phases 1-4 (which it isn't :)) then we would
expect each comment to be transformed to a single space character.  So
this looks like a good fix for portability.

> (also it would be nice to include a configure script in
> the git repo,

No, it would not be nice.

> or don't use autoconf in the first place)

The nice things automake offers for dash are support for
cross-compilation, dependency detection using gcc -MD, and VPATH
builds.  I am a nobody, so my opinion does not mean much, but: I would
be very happy to use a plain makefile that can do those things, too.

In that imagined scheme, the configure script would stick to what it
is best at --- detecting information about the platform and
configuration.  The configure script output could go in a makefile
fragment that the makefile uses with an "include" directive (relying
on sane defaults when it is not present).  The git project is an
example of this (though missing features like cross-compilation and
VPATH builds).

I'd be happy to help out with that (as an alternate build system at
first).

Thanks and hope that helps.

-- >8 --
Subject: [OUTPUT] Make outc an inline function

As "gcc -pedantic" warns, ISO C forbids conditional expressions with
only one void side.  So the (needslow ?  slowpath() : fastpath) code
for outc in the !USE_GLIBC_STDIO case might not be portable.

More importantly, it's hard to read.  Rip it out and replace it
with an inline function which should generate the same code.

Reported-by: Szabolcs Nagy <nsz@port70.net>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Not even compile-tested.

 src/output.h |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/output.h b/src/output.h
index d123301..4632329 100644
--- a/src/output.h
+++ b/src/output.h
@@ -97,10 +97,21 @@ freestdout()
 #define OUTPUT_ERR 01		/* error occurred on output */
 
 #ifdef USE_GLIBC_STDIO
-#define outc(c, o)	putc((c), (o)->stream)
+static inline void outc(int ch, struct output *file)
+{
+	putc(ch, file->stream);
+}
 #define doformat(d, f, a)	vfprintf((d)->stream, (f), (a))
 #else
-#define outc(c, file)	((file)->nextc == (file)->end ? outcslow((c), (file)) : (*(file)->nextc = (c), (file)->nextc++))
+static inline void outc(int ch, struct output *file)
+{
+	if (file->nextc != file->end) {
+		*file->nextc = ch;
+		file->nextc++;
+		return;
+	}
+	outcslow(ch, file);
+}
 #endif
 #define out1c(c)	outc((c), out1)
 #define out2c(c)	outcslow((c), out2)
-- 
1.7.5.1

Re: build issues when using pcc

[Jonathan Nieder]

Szabolcs Nagy wrote:
> * Jonathan Nieder <jrnieder@gmail.com> [2011-05-22 11:46:05 -0500]:

>> What cpp do you use (so others can catch similar problems in the
>> future)?
>
> pcc uses its own preprocessor

Ah, sloppy me.  I stupidly read pcc as "ppc" but you are talking
about http://pcc.ludd.ltu.se/

Will check it out; thank you.

Re: build issues when using pcc

[Herbert Xu]

On Sun, May 22, 2011 at 04:46:05PM +0000, Jonathan Nieder wrote:
>
> Subject: [OUTPUT] Make outc an inline function
> 
> As "gcc -pedantic" warns, ISO C forbids conditional expressions with
> only one void side.  So the (needslow ?  slowpath() : fastpath) code
> for outc in the !USE_GLIBC_STDIO case might not be portable.
> 
> More importantly, it's hard to read.  Rip it out and replace it
> with an inline function which should generate the same code.
> 
> Reported-by: Szabolcs Nagy <nsz@port70.net>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Patch applied.

I've also added this one-liner on top to eliminate an unnecessary
promotion caused by this patch.

commit 155b0b518f485b8b5cb574cd2f0dc936c3fd1902
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date:   Thu Jul 7 15:22:03 2011 +0800

    [BUILTIN] Eliminate unnecessary promotion in echocmd
    
    The patch to make outc into an inline function created an unnecessary
    promotion in echocmd due to its use of char vs. the int used by outc.
    This patch changes echocmd to use int instead.
    
    Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/ChangeLog b/ChangeLog
index ade43e9..2093fc7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,6 +1,7 @@
 2011-07-07  Herbert Xu <herbert@gondor.apana.org.au>
 
 	* Optimize dash -c "command" to avoid a fork.
+	* Eliminate unnecessary promotion in echocmd.
 
 2011-05-22  Jonathan Nieder <jrnieder@gmail.com>
 
diff --git a/src/bltin/printf.c b/src/bltin/printf.c
index b0c3774..893295c 100644
--- a/src/bltin/printf.c
+++ b/src/bltin/printf.c
@@ -443,7 +443,7 @@ echocmd(int argc, char **argv)
 	}
 
 	do {
-		char c;
+		int c;
 
 		nonl += conv_escape_str(*argv);
 		outstr(stackblock(), outs);

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt