`SCM_MAKE_CHAR ()' signedness issue

`SCM_MAKE_CHAR ()' signedness issue

[Ludovic Courtès]

Hello,

We still have troubles with the `(scm_t_int32) (x) < 0' test in
`SCM_MAKE_CHAR ()':

--8<---------------cut here---------------start------------->8---
ludo@gcc54:~/guile-1.9.1/+build$ cat ,,t.c
int
foo (unsigned char x)
{
  return (((int)x) < 0 ? 1 : -1);
}
ludo@gcc54:~/guile-1.9.1/+build$ gcc -Wall -c ,,t.c
,,t.c: In function ‘foo’:
,,t.c:4: warning: comparison is always false due to limited range of data type
ludo@gcc54:~/guile-1.9.1/+build$ ../build-aux/config.guess 
sparc64-unknown-linux-gnu
ludo@gcc54:~/guile-1.9.1/+build$ gcc --version
gcc (GCC) 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)
Copyright (C) 2006 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
--8<---------------cut here---------------end--------------->8---

The problem occurs, e.g., in `string_titlecase_x ()'.

Surprisingly, GCC 4.4 on x86_64-linux-gnu doesn't yield any warning for
the example above.

I'm not sure how to fix it, so we'll see for the next release.

Thanks,
Ludo'.

Re: `SCM_MAKE_CHAR ()' signedness issue

[Ken Raeburn]

On Aug 15, 2009, at 08:00, Ludovic Courtès wrote:
> Hello,
>
> We still have troubles with the `(scm_t_int32) (x) < 0' test in
> `SCM_MAKE_CHAR ()':
>
> --8<---------------cut here---------------start------------->8---
> ludo@gcc54:~/guile-1.9.1/+build$ cat ,,t.c
> int
> foo (unsigned char x)
> {
>  return (((int)x) < 0 ? 1 : -1);
> }
> ludo@gcc54:~/guile-1.9.1/+build$ gcc -Wall -c ,,t.c
> ,,t.c: In function ‘foo’:
> ,,t.c:4: warning: comparison is always false due to limited range of  
> data type

In the case of SCM_MAKE_CHAR, both actions give identical results for  
an input of 0, so I think just testing "x <= 0" will silence the  
warning without changing the behavior.

There's always the inline-function approach, too.

Ken

Re: `SCM_MAKE_CHAR ()' signedness issue

[Ludovic Courtès]

Ken Raeburn <raeburn@raeburn.org> writes:

> In the case of SCM_MAKE_CHAR, both actions give identical results for
> an input of 0, so I think just testing "x <= 0" will silence the
> warning without changing the behavior.

Oh, good point.

> There's always the inline-function approach, too.

Unfortunately no, because we're still not assuming `inline' keyword
support from the compiler.

Thanks,
Ludo'.

Re: `SCM_MAKE_CHAR ()' signedness issue

[Ken Raeburn]

On Aug 16, 2009, at 18:13, Ludovic Courtès wrote:
>> There's always the inline-function approach, too.
>
> Unfortunately no, because we're still not assuming `inline' keyword
> support from the compiler.

Right, but inline.h deals with that; if "inline" isn't supported you  
just get a declaration and make a function call.  There would be a  
performance hit from doing the function calls all the time, but I  
think it would work.  In fact, there are cases where the argument to  
SCM_MAKE_CHAR is a function invocation, where it might be beneficial  
for performance to not compute the value twice.

Ken

Re: `SCM_MAKE_CHAR ()' signedness issue

[Ludovic Courtès]

Hi,

Ken Raeburn <raeburn@raeburn.org> writes:

> On Aug 16, 2009, at 18:13, Ludovic Courtès wrote:
>>> There's always the inline-function approach, too.
>>
>> Unfortunately no, because we're still not assuming `inline' keyword
>> support from the compiler.
>
> Right, but inline.h deals with that; if "inline" isn't supported you
> just get a declaration and make a function call.  There would be a
> performance hit from doing the function calls all the time,

Yes, I'm not sure that's something worth trying.

> In fact, there are cases where the argument to SCM_MAKE_CHAR is a
> function invocation, where it might be beneficial for performance to
> not compute the value twice.

Right, but I guess these could be avoided trivially.

Thanks,
Ludo'.

Re: `SCM_MAKE_CHAR ()' signedness issue

[Mike Gran]

Hi-

On Mon, 2009-08-17 at 10:26 +0200, Ludovic Courtès wrote: 
> Hi,
> 
> Ken Raeburn <raeburn@raeburn.org> writes:
> 
> > On Aug 16, 2009, at 18:13, Ludovic Courtès wrote:
> >>> There's always the inline-function approach, too.
> >>
> >> Unfortunately no, because we're still not assuming `inline' keyword
> >> support from the compiler.
> >
> > Right, but inline.h deals with that; if "inline" isn't supported you
> > just get a declaration and make a function call.  There would be a
> > performance hit from doing the function calls all the time,
> 
> Yes, I'm not sure that's something worth trying.

On my system I ran a test with SCM_MAKE_CHAR as a macro, an an inline,
and as a never inlined function.  I ran ./check-guile twice for each.

SCM_MAKE_CHAR as macro, ./check-guile gives
real 0m22.680s 0m22.658s
user 0m7.700s  0m7.640s
sys 0m1.067s  0m1.124s

SCM scm_i_make_char (scm_t_int32 x) __attribute__((noinline))
real 0m22.010s 0m21.998s
user 0m7.631s  0m7.648s
sys 0m1.151s  0m1.076s

SCM inline scm_i_make_char (scm_t_int32 x)
real 0m22.107s 0m21.914s
user 0m7.614s  0m7.726s
sys 0m1.115s  0m1.068s

The timing differences between them seem to be in the noise, for this
one test.

Thanks,

Mike

Re: `SCM_MAKE_CHAR ()' signedness issue

[Ludovic Courtès]

Mike Gran <spk121@yahoo.com> writes:

> On my system I ran a test with SCM_MAKE_CHAR as a macro, an an inline,
> and as a never inlined function.  I ran ./check-guile twice for each.

You're cheating!  ;-)

The test suite does lots of things besides calling `SCM_MAKE_CHAR ()',
so I'm not sure if it's a good benchmark.

I'm fairly confident that for such a small piece of code inlining is
always a good idea.

Thanks,
Ludo'.

Re: `SCM_MAKE_CHAR ()' signedness issue

[carlo.bramix]

Hello,
if I understood the problem, I think it can be easily fixed without using an inline function.

For example:

#ifdef __GNUC__

#define SCM_MAKE_CHAR(__x)                                            \
({ scm_t_int32 __t = (scm_t_int32)(__x);                              \
  (__t < 0)                                                           \
  ? SCM_MAKE_ITAG8 ((scm_t_bits) (unsigned char) (__x), scm_tc8_char) \
  : SCM_MAKE_ITAG8 ((scm_t_bits) (__x), scm_tc8_char);                \
  __t;                                                                \
})

#else

#define SCM_MAKE_CHAR(x)                                              \
  ((scm_t_int32) (x) < 0                                              \
  ? SCM_MAKE_ITAG8 ((scm_t_bits) (unsigned char) (x), scm_tc8_char)   \
  : SCM_MAKE_ITAG8 ((scm_t_bits) (x), scm_tc8_char))

#endif

solves the problem.
Actually a much more efficient solution specifically designed for GCC may use __builtin_types_compatible_p() and __builtin_choose_expr() intrinsic functions, but this will force you to support GCC 3.x or newer.

Something like:
#if defined __GNUC__ && __GNUC__ >= 3
...
#elif ...

will need to be written.

Sincerely,

Carlo Bramini.

---------- Initial Header -----------

From      : guile-devel-bounces+carlo.bramix=libero.it@gnu.org
To          : guile-devel@gnu.org
Cc          :
Date      : Mon, 17 Aug 2009 17:33:03 +0200
Subject : Re: `SCM_MAKE_CHAR ()' signedness issue

> Mike Gran <spk121@yahoo.com> writes:
>
> > On my system I ran a test with SCM_MAKE_CHAR as a macro, an an inline,
> > and as a never inlined function.  I ran ./check-guile twice for each.
>
> You're cheating!  ;-)
>
> The test suite does lots of things besides calling `SCM_MAKE_CHAR ()',
> so I'm not sure if it's a good benchmark.
>
> I'm fairly confident that for such a small piece of code inlining is
> always a good idea.
>
> Thanks,
> Ludo'.
>

Re: `SCM_MAKE_CHAR ()' signedness issue

[Ken Raeburn]

On Aug 17, 2009, at 14:52, carlo.bramix wrote:
> Hello,
> if I understood the problem, I think it can be easily fixed without  
> using an inline function.
>
> For example:
>
> #ifdef __GNUC__

Relying on GCC-specific syntax is probably worse than an inline  
function.  Part of Ludovic's complaint was that we don't want to  
depend on "inline" working in some random compiler; but then, we  
certainly shouldn't depend on statement expressions working everywhere  
either.  And if you're going to conditionalize GCC and non-GCC  
versions, you might as well use an inline function in the GCC case,  
especially since other C99 compilers should support it as well.  The  
machinery in inline.h is set up to deal with that, though it chooses  
between inline expansion and function calls, not between inline  
function expansion and macro expansion.  I think we can probably argue  
that if inline function expansion is disabled for some reason in a  
compiler that does support it, performance is not a key issue;  
probably "-O" was omitted on the command line or something.  So the  
thought of falling back to a function call in that case really doesn't  
bother me.

(I have written code to rely on GCC-specific extensions before, and  
code that conditionalizes such things. I think there are cases where  
you may want to do that.  I just don't think this is one of them.)

The only benefits I see here to going with the macro are (1) minor  
performance differences on non-C99, non-GCC compilers, (2) different  
semantics if some odd type -- like a 64-bit value -- is provided, (3)  
simpler code, if you just have one version of the macro.

Ken

Re: `SCM_MAKE_CHAR ()' signedness issue

[Mike Gran]

> From: Ludovic Courtès <ludo@gnu.org>
> To: guile-devel@gnu.org
> Sent: Monday, August 17, 2009 8:33:03 AM
> Subject: Re: `SCM_MAKE_CHAR ()' signedness issue
> 
> I'm fairly confident that for such a small piece of code inlining is
> always a good idea.

OK.  If the comparison is modified to become

35 #define SCM_MAKE_CHAR(x)                                               \
36   (((x) < 128)                                               \
37    ? SCM_MAKE_ITAG8 ((scm_t_bits) (unsigned char) (x), scm_tc8_char)   \
38    : SCM_MAKE_ITAG8 ((scm_t_bits) (x), scm_tc8_char))

Then hopefully that will solve all the build problems.

> Thanks,
> Ludo'.

-Mike

Re: `SCM_MAKE_CHAR ()' signedness issue

[carlo.bramix]

Hello,
unfortunately that code still fails into libguile/print.c
Infact, a signed char just arrives to 127 and the " < 128" causes:

../../guile-git/libguile/print.c:1101: warning: comparison is always true due to limited range of data type
../../guile-git/libguile/print.c:1108: warning: comparison is always true due to limited range of data type

But in this manner it seems ok:

#define SCM_MAKE_CHAR(x)                                               \
  (((scm_t_int32) (x) + 128) < 128                                     \
   ? SCM_MAKE_ITAG8 ((scm_t_bits) (unsigned char) (x), scm_tc8_char)   \
   : SCM_MAKE_ITAG8 ((scm_t_bits) (x), scm_tc8_char))

Sincerely,

Carlo Bramini.

---------- Initial Header -----------

From      : guile-devel-bounces+carlo.bramix=libero.it@gnu.org
To          : "Ludovic Courtès" ludo@gnu.org, guile-devel@gnu.org
Cc          :
Date      : Tue, 18 Aug 2009 10:32:29 -0700 (PDT)
Subject : Re: `SCM_MAKE_CHAR ()' signedness issue

> > From: Ludovic Courtès <ludo@gnu.org>
> > To: guile-devel@gnu.org
> > Sent: Monday, August 17, 2009 8:33:03 AM
> > Subject: Re: `SCM_MAKE_CHAR ()' signedness issue
> >
> > I'm fairly confident that for such a small piece of code inlining is
> > always a good idea.
>
> OK.  If the comparison is modified to become
>
> 35 #define SCM_MAKE_CHAR(x)                                               \
> 36   (((x) < 128)                                               \
> 37    ? SCM_MAKE_ITAG8 ((scm_t_bits) (unsigned char) (x), scm_tc8_char)   \
> 38    : SCM_MAKE_ITAG8 ((scm_t_bits) (x), scm_tc8_char))
>
> Then hopefully that will solve all the build problems.
>
> > Thanks,
> > Ludo'.
>
> -Mike
>
>
>
>

Re: `SCM_MAKE_CHAR ()' signedness issue

[Mike Gran]

> From: carlo.bramix <carlo.bramix@libero.it>
> 
> Hello,
> unfortunately that code still fails into libguile/print.c
> Infact, a signed char just arrives to 127 and the " < 128" causes:
> 
> ../../guile-git/libguile/print.c:1101: warning: comparison is always true due to 
> limited range of data type
> ../../guile-git/libguile/print.c:1108: warning: comparison is always true due to 
> limited range of data type

Doh!  You're right.

> 
> But in this manner it seems ok:
> 
> #define SCM_MAKE_CHAR(x)                                              \
>   (((scm_t_int32) (x) + 128) < 128                                    \
>   ? SCM_MAKE_ITAG8 ((scm_t_bits) (unsigned char) (x), scm_tc8_char)  \
>   : SCM_MAKE_ITAG8 ((scm_t_bits) (x), scm_tc8_char))

Alternately, you could put in any number between 0 and 127, I think, and get 
the correct behavior.

#define SCM_MAKE_CHAR(x)                                              \
  ((x) < 64                                    \
  ? SCM_MAKE_ITAG8 ((scm_t_bits) (unsigned char) (x), scm_tc8_char)  \
  : SCM_MAKE_ITAG8 ((scm_t_bits) (x), scm_tc8_char))

-Mike

Re: `SCM_MAKE_CHAR ()' signedness issue

[Greg Troxel]

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


I think the problem is that the routine in question is fundamentally
written to assume a signed input.  Perhaps there should be an unsigned
version that takes unsigned chars, and perhaps guile should explicit
declare characters as one or the other.  Or perhaps just always cast to
unsigned char - if it is already unsiged that is a no-op.



[-- Attachment #2: Type: application/pgp-signature, Size: 193 bytes --]