Subtle error defining VALMASK?

Subtle error defining VALMASK?

[Dmitry Antipov]

In:

#define VALMASK_val (USE_LSB_TAG ? - (1 << GCTYPEBITS) : VAL_MAX)

shouldn't it be

#define VALMASK_val (USE_LSB_TAG ? - (1L << GCTYPEBITS) : VAL_MAX)

or, if --with-wide-int on a 32-bit system:

#define VALMASK_val (USE_LSB_TAG ? - (1LL << GCTYPEBITS) : VAL_MAX)

?

Dmitry

Re: Subtle error defining VALMASK?

[Paul Eggert]

Dmitry Antipov wrote:
> In:
>
> #define VALMASK_val (USE_LSB_TAG ? - (1 << GCTYPEBITS) : VAL_MAX)
>
> shouldn't it be
>
> #define VALMASK_val (USE_LSB_TAG ? - (1L << GCTYPEBITS) : VAL_MAX)
>
> or, if --with-wide-int on a 32-bit system:
>
> #define VALMASK_val (USE_LSB_TAG ? - (1LL << GCTYPEBITS) : VAL_MAX)

There's no error here.  All three definitions are equivalent because - 
(1 << GCTYPEBITS) equals -8, which sign-extends to the width of VAL_MAX. 
  When it doesn't matter, it's better to avoid length suffixes on 
integer constants.

There is another programming style, where authors take care to write 
exact suffixes on constants, e.g., 'lseek (fd, 0L, SEEK_SET)' when 
lseek's 2nd argument is 'long'.  In the old days before function 
prototypes this style was often necessary but that need went away long 
ago, and nowadays this other style is typically more trouble than it's 
worth.  For example, the 'lseek' call would be "wrong" now because 
lseek's argument type was changed from 'long' to 'off_t', and so a 
maintainer would have to change the call to 'lseek (fd, (off_t) 0, 
SEEK_SET)', whereas the simpler style 'lseek (fd, 0, SEEK_SET)' would 
survive the API change without needing maintenance.

Re: Subtle error defining VALMASK?

[Dmitry Antipov]

On 09/10/2014 06:58 PM, Paul Eggert wrote:

> There's no error here.  All three definitions are equivalent because - (1 << GCTYPEBITS) equals -8,
> which sign-extends to the width of VAL_MAX.

fprintf (stderr, "0x%lx 0x%lx\n", VALMASK, VAL_MAX);

==>

../../trunk/src/alloc.c: In function ‘init_alloc’:
../../trunk/src/alloc.c:7240:3: error: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 3 has type ‘int’ [-Werror=format=]
    fprintf (stderr, "0x%lx 0x%lx\n", VALMASK, VAL_MAX);
    ^
cc1: all warnings being treated as errors

???

Dmitry

Re: Subtle error defining VALMASK?

[Paul Eggert]

Dmitry Antipov wrote:
> fprintf (stderr, "0x%lx 0x%lx\n", VALMASK, VAL_MAX);

That's not a problem with the #define; it's a problem with the enum. 
It's simpler to omit the enum, so I did that in trunk bzr 117854.

For portability that format string should be "0x%"pI"x 0x%"pI"x\n".

More issues with r117851 [Was: Re: Subtle error defining VALMASK?]

[Dmitry Antipov]

On 09/10/2014 08:54 PM, Paul Eggert wrote:

> That's not a problem with the #define; it's a problem with the enum.
> It's simpler to omit the enum, so I did that in trunk bzr 117854.

Thanks.

But there is one more issue with r117851.  Even after an obvious fix:

=== modified file 'src/lisp.h'
--- src/lisp.h	2014-09-10 16:52:50 +0000
+++ src/lisp.h	2014-09-10 17:20:29 +0000
@@ -4590,7 +4590,7 @@
  /* Return a function-scoped string with contents DATA and length NBYTES.  */

  # define make_local_string(data, nbytes) \
-    make_local_string (data, nbytes, __COUNTER__)
+    make_local_string_n (data, nbytes, __COUNTER__)
  # define make_local_string_n(data_arg, nbytes_arg, n)			\
      ({									\
         char const *data##n = data_arg;					\
@@ -4599,7 +4599,7 @@
         if (nbytes##n <= MAX_ALLOCA - sizeof (struct Lisp_String) - 1)	\
  	 {								\
  	   struct Lisp_String *ptr##n					\
-	     = alloca (sizeof (struct Lisp_String) + 1 + nbytes);	\
+	     = alloca (sizeof (struct Lisp_String) + 1 + nbytes##n);	\
  	   string##n = local_string_init (ptr##n, data##n, nbytes##n);	\
  	 }								\
         else								\


__COUNTER__ is not expanded, e.g. this:

=== modified file 'src/editfns.c'
--- src/editfns.c	2014-09-07 07:04:01 +0000
+++ src/editfns.c	2014-09-10 17:15:08 +0000
@@ -1318,7 +1318,7 @@
    p = USER_FULL_NAME;
    /* Chop off everything after the first comma. */
    q = strchr (p, ',');
-  full = make_string (p, q ? q - p : strlen (p));
+  full = make_local_string (p, q ? q - p : strlen (p));

  #ifdef AMPERSAND_FULL_NAME
    p = SSDATA (full);

expands to:

   p = pw->pw_gecos;
   /* Chop off everything after the first comma. */
   q = strchr (p, ',');
   full = ({ char const *data__COUNTER__ = p; ptrdiff_t nbytes__COUNTER__ = q ? q - p : strlen (p); Lisp_Object string__COUNTER__; if (nbytes__COUNTER__ <= MAX_ALLOCA - sizeof (struct Lisp_String) - 
1) { struct Lisp_String *ptr__COUNTER__ = __builtin_alloca (sizeof (struct Lisp_String) + 1 + nbytes__COUNTER__); string__COUNTER__ = local_string_init (ptr__COUNTER__, data__COUNTER__, 
nbytes__COUNTER__); } else string__COUNTER__ = make_string (data__COUNTER__, nbytes__COUNTER__); string__COUNTER__; });

Dmitry

Re: More issues with r117851 [Was: Re: Subtle error defining VALMASK?]

[Paul Eggert]

On 09/10/2014 10:25 AM, Dmitry Antipov wrote:
>
> __COUNTER__ is not expanded

Thanks, on second thought the __COUNTER__ business was overkill here so 
I removed it in trunk bzr 117859.