Build failure on debian

[Lee Bieber]

Stewart's 64bit Debian machine is failing to compile. Not sure exactly when this started as it has been awhile since we have been able to compile on this machine

See the complete log at http://solace.inaugust.com/builders/debian.sparc.64.1/builds/27/steps/compile/logs/stdio

libtool: compile: g++ -DHAVE_CONFIG_H -I. -I. -ggdb3 -pthread -pipe -O3 -Werror -pedantic -Wall -Wextra -Wundef -Wshadow -fdiagnostics-show-option -fvisibility=hidden -Wformat -fno-strict-aliasing -Wno-strict-aliasing -Woverloaded-virtual -Wnon-virtual-dtor -Wctor-dtor-privacy -Wno-long-long -Wmissing-declarations -Wno-redundant-decls -std=gnu++0x -MT plugin/heap/hp_block.lo -MD -MP -MF plugin/heap/.deps/hp_block.Tpo -c plugin/heap/hp_block.cc -o plugin/heap/hp_block.o >/dev/null 2>&1
cc1plus: warnings being treated as errors
plugin/command_log/command_log.cc: In member function ‘virtual void CommandLog::apply(drizzled::message::Command*)’:
plugin/command_log/command_log.cc:161: error: right shift count >= width of type

[Jay Pipes (jaypipes)]

Not sure what is going on here. It's a call to one of the lovely korr.h macros:

  int8store(nbo_length, length);

nbo_length is declared as unsigned char[8] and length is declared as size_t.

This macros is variously #defined, based on arch and byte-alignment as:

i386:

#define int8store(T,A) *((uint64_t *) (T))= (uint64_t) (A)

SPARC:

#define int8store(T,A) do { uint32_t def_temp= (uint32_t) (A), def_temp2= (uint32_t) ((A) >> 32); \
                                  int4store((T),def_temp); \
                                  int4store((T+4),def_temp2); } while(0)

Clearly, the issue here is that a size_t -- the length variable -- is being bit-shifted 32 bits to the right. On all *sane* platforms, a size_t is unsigned, either 32 or 64 bit integers. Of course, on SPARC, it's not. It's a signed integer, and shifting 32 bits goes beyond the signed integer boundary.

Short of fixing SPARC to be not-stoopid, the solution to this is to use an uint32_t instead of a size_t, and then static_cast<size_t>(length) in all the cases (most of them) where length is needed in GPB calls.

I will fix this shortly.

Screw SPARC. :)

-jay

[Jay Pipes (jaypipes)]

Another way to solve this is to have the macro:

#define int8store(T,A) do { uint32_t def_temp= (uint32_t) (A), def_temp2= (uint32_t) ((A) >> 32); \
                                  int4store((T),def_temp); \
                                  int4store((T+4),def_temp2); } while(0)

changed instead to this:

#define int8store(T,A) do { \
                                  uint32_t def_temp= static_cast<uint32_t>((A)); \
                                  uint32_t def_temp2= static_cast<uint32_t>((A)) >> 32; \
                                  int4store((T),def_temp); \
                                  int4store((T+4),def_temp2); \
                                  } while(0)

Brian, Monty, thoughts? Should I change this macro definition instead of changing the command_log.cc code?

[Stewart Smith (stewart)]

This is also on 32bit Linux PowerPC:

http://solace.inaugust.com/builders/debian.32.1/builds/94/steps/compile/logs/stdio

I think the correct solution here is to build so that size_t is 64bit on these platforms (e.g. defining _LARGEFILE64_SOURCE)

[Monty Taylor (mordred)]

Stewart Smith wrote:
> This is also on 32bit Linux PowerPC:
>
> http://solace.inaugust.com/builders/debian.32.1/builds/94/steps/compile/logs/stdio
>
> I think the correct solution here is to build so that size_t is 64bit on
> these platforms (e.g. defining _LARGEFILE64_SOURCE)
>

For the record, from C99:

(regarding sizeof operator)
4 The value of the result is implementation-defined, and its type (an
unsigned integer type)
  is size_t, defined in <stddef.h> (and other headers).

and then:

  7.17 Common definitions <stddef.h>
1 The following types and macros are defined in the standard header
<stddef.h>. Some
  are also defined in other headers, as noted in their respective
subclauses.
2 The types are
          ptrdiff_t
  which is the signed integer type of the result of subtracting two
pointers;
          size_t
  which is the unsigned integer type of the result of the sizeof operator;

So, if Solaris or Debian Sparc is defining size_t as a signed int, then
it's a bug in the header. I find it hard to believe that debian sparc
defines size_t differently than debian non-sparc, since that header
should be coming from glibc, but stranger things have happened. Can you
point me towards something that's showing size_t == signed?

Monty

[Stewart Smith (stewart)]

On Tue, Sep 22, 2009 at 09:24:15AM -0000, Monty Taylor wrote:
> Stewart Smith wrote:
> > This is also on 32bit Linux PowerPC:
> >
> > http://solace.inaugust.com/builders/debian.32.1/builds/94/steps/compile/logs/stdio
> >
> > I think the correct solution here is to build so that size_t is 64bit on
> > these platforms (e.g. defining _LARGEFILE64_SOURCE)
> >

Just looking back at this... either the use of int8store is incorrect or
the use of size_t itself is incorrect (and it should be an off_t/off64_t).

I am, of course, going to blame some kind of delayed jetlag for
previously mentioning size_t and a compile flag. That was just... stupid.
--
Stewart Smith

[Jay Pipes (jaypipes)]

Fix didn't work...back to drawing board.