Re: [Libreoffice] debug build fails because graphite ./configure not up to date

On Fri, Jan 14, 2011 at 7:44 PM, Lionel Elie Mamane <lionel@mamane.lu> wrote:
> On Sat, Jan 15, 2011 at 01:38:34AM +0100, Christian Lohmaier wrote:
> > On Sat, Jan 15, 2011 at 12:43 AM, Lionel Elie Mamane <lionel@mamane.lu> wrote:
>
> > > The problem is that graphite does not build with -Werror in debug
> > > mode; one of the least fixable reasons is that it uses asserts left
> > > and right, asserts are *designed* to always be true, sometimes gcc
> > > detects that they are (e.g. when comparing an unsigned integer type
> > > with 0), this raises a warning that gets treated as error because
> > > of -Werror.
>
> > Sorry - but when checking whether an unsigned type is larger or
> > equals zero, then this is a bug in the assertion and the compiler
> > rightfully complains about stupid code that could be written as
> > "true" instead.
>
> Well, not necessarily. The type is unsigned on the OS/architecture
> being currently compiled for, but not necessarily on all
> OS/architectures. E.g. size_t was signed on some older unices.

According to the 1999 ISO C standard (C99), size_t is an _unsigned_
integer type of at least 16 bits.
It's going to be very hard to write portable code if we assume that
the target do not follow the standard and can't be beaten in
compliance with some header hackery...


> Or
> "char", which is allowed by the C and C++ standards to be either
> signed or unsigned.
> E.g. in case you use "char" as a "tiny int" for
> values between 0 and 10, you'd assert "value >=0 && value <= 10".

That's a bug. you should use int8_t or uint8_t for that purpose (or
some equivalent non-ambiguous typedef), never char
or you are going to have bugs. for the very reason you mentioned that
the signed nature of char is unspecified.

> Or also, when defining a preprocessor macro: the macro can be used
> with _any_ type, signed or unsigned. When the type is unsigned (part
> of) what it does is useless, but hey, the compiler will optimise it
> away. And the test makes sense were the macro is used with a signed
> type. =
>
> > But maybe I misunderstand... Do you have an example?
>
> graphite file engine/include/graphite/GrDebug.h:
>
>  #define AssertPtrSize(pv, cb) Assert((cb) >= 0 && ValidReadPtrSize((pv), cb))
>
> graphite file: engine/src/generic/Util.cpp:
>
> Macro used the first time with a signed type in second argument:
>
> void SwapBytes(void * pv1, void * pv2, int cb)
> {
>        AssertPtrSize(pv1, cb);
>        AssertPtrSize(pv2, cb);
>
> And now with an unsigned type:
>
> void FillShorts(void * pv, short sn, int csn)
> {
>        AssertPtrSize(pv, csn * isizeof(short));
>
> It makes total sense that the macro compares to zero, else it would
> not catch some assert-able error in the first case!
>
> Now that I look more closely at it, I see (in GrCommon.h) the authors intended
> to address that issue:
>
> // This is to make a signed isizeof operator, otherwise we get tons of warnings about
> // signed / unsigned mismatches.
> #define isizeof(T) (sizeof(T))
>
> Well, that ain't gonna work, it is not making any cast... Maybe
> something like
>
> #define isizeof(T) ((ssize_t) sizeof(T))
>
> would work (but maybe loose some portability to
> OS/architectures/compilers that don't define ssize_t.

That is not too much of a problem. ssize_t can be defined for those
platform if any.

I think the morale of this argument is:
If you want to write portable code, don't rely on non-portable construct.
otherwise you're going to have warning at best, bug at worse on some platforms.

> Or possibly
>
> #define isizeof(T) ((ptrdiff_t) sizeof(T))
>
> would be available "everywhere" and *maybe* safe (as in ptrdiff_t will
> always be big enough).

Or: don't use AssertPtrSize(a,b) when it is not appropriate:
for instance
the
 AssertPtrSize(pv, csn * isizeof(short));
can be coded
 AssertPtr(pv) && assert(csn >= 0)
or
 AssertPtrSize(pv, csn)  (which achieve the same intended result - the
original assert was clearly not there to detect overflow)

then of course there is the hammer solution:
#define AssertPtrSize(pv, cb) Assert(((ssize_t)(cb)) >= 0 &&
ValidReadPtrSize((pv), cb))

you hide everything under the carpet all the same, but at least you
change only one line, not all the call-site.

Norbert

> --
> Lionel
> _______________________________________________
> LibreOffice mailing list
> LibreOffice@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libreoffice