Re: Compiler warnings in psqloodbc 08.03.0200

Hi,

Hiroshi Saito írta:
> Hi Zoltan-san.
>
> I appreciate much work. Please let me late review by the reason for
> not having margin
> time now.

No problem, take your time.
I just wanted it to be out of my machine. :-)

> sorry. However, we did those checks and adjustments in much environment.
> They are VISTA-32bit, winXP-32bit, win2008-64bit,RedHat x86_64,
> FreeBSD-32bit,
> Furthermore, VC6, VC8, Access2000,Access-XP,2005, Cygwin and etc
> required serious time....
> Anyway, As for the Ver 08.03.0300, the result of those tests is
> reflected.

I understand your position. I just checked on Visual C++ 2005
and 2008 and they also understand __FUNCTION__ and it works
as expected, i.e. it presents the current function name just as on GCC.
The Microsoft OS doesn't matter, it's the compiler that matters.
I suspect things like __FUNCTION__ are actually defined in the
C language standard as a mandatory feature provided by the compiler
and *BSDs also conform.

However, on some systems (newer Linux systems with a recent
GCC 4.x) spit out "pointers differ in signedness" warnings if the
signedness of char variables/expressions differ in assignments or
passed-in vs declared function parameters. As Tom said, this can
create a situation where there are so many of them that other more
serious warnings are simply lost in the noise. A zero-warning policy
is indeed useful in C language projects.

> BTW, the description document point of nptl was unknown...
> --

The below is not about NPTL workings, it's about GLIBC in general
on Fedora 9 and most other GLIBC-using systems, like your
RedHat x86_64 above. In pthread.h, pthread_mutexattr_settype() and
others are protected inside

#ifdef __USE_UNIX98
...
#endif

So simply compiling it produces a "implicit declaration of //function '...'"
warning. __USE_UNIX98 gets defined in features.h if the symbol
_XOPEN_SOURCE is defined with a value >=500. features.h is
included by all other GLIBC headers directly or indirectly.
But defining _XOPEN_SOURCE seems to override the GCC
default (POSIX, XOPEN, etc) conformance level symbols and
now strcasecmp(), strncasecmp(), snprintf() and malloc() are
undefined now. To make them visible again, _BSD_SOURCE or
_GNU_SOURCE has to be defined. I chose _BSD_SOURCE
because it will not offend people using BSDs. :-)

>> 2) Fix for "implicit declaration of pthread_mutexattr_settype"
>> on my Fedora 9 system.
> --

Best regards,
Zoltán Böszörményi

> Thanks.
>
> Regards,
> Hiroshi Saito
>
> ----- Original Message ----- From: "Zoltan Boszormenyi" <zb(at)cybertec(dot)at>
>
>
>> Hi,
>>
>> here are two patches that are applicable after my previous patch.
>> 1) Cleanup to replace CSTR func = "..." with __FUNCTION__.
>> There will be no more "'func' is defined but not used."
>> This was done by sed scripts and verified manually
>> so only the relevant places were substituted.
>> 2) Fix for "implicit declaration of pthread_mutexattr_settype"
>> on my Fedora 9 system.
>>
>> Adding "-Wno-pointer-sign" to AM_CFLAGS would
>> prevent "pointer differ in signedness" warnings but
>> it would cover an error in erroneous 8-bit arithmetics.
>> Cleaning up the SQLCHAR vs. char problems is preferred.
>>
>> Best regards,
>> Zoltán Böszörményi
>>
>> Zoltan Boszormenyi írta:
>>> Zoltan Boszormenyi írta:
>>>
>>>> Hi,
>>>>
>>>> here's the fix for all non-pointer-signedness warnings,
>>>> against 08.03.0300 that was released meanwhile. Now
>>>> the compilation only emits 246 "differ in signedness"
>>>> warnings, which is still too much noise. I agree with
>>>> Tom Lane that those should be cleaned up if for nothing
>>>> else, than the real bugs don't get lost in the noise.
>>>>
>>>> The patch cleans up such warnings in several files:
>>>> "label 'cleanup' defined but not used" and
>>>> "unused variable 'func'"
>>>>
>>>> In convert.c::convert_escape():
>>>> "'param_consumed' may be used uninitialized in this function"
>>>> The variable "param_consumed" was not assigned a value
>>>> in all cases by processParameters() upon returning,
>>>> so I tried to fix it there. Now it does, please review.
>>>>
>>>> In descriptor.c::TI_Constructor():
>>>> "the address of 'qual' will always evaluate as 'true'"
>>>> STRX_TO_NAME() has to be used instead of STR_TO_NAME.
>>>>
>>>> In drvconn.c::dconn_get_attributes():
>>>> "'last' may be used uninitialized in this function"
>>>> The first parameter to strtok_r() may be NULL if
>>>> strdup() returns NULL. In that case strtok_r() may
>>>> corrupt unknown memory areas.
>>>>
>>>> In pgapi30.c, two instances of
>>>> "cast from pointer to integer of different size"
>>>>
>>>> In psqlodbc.c()::finalize_global_cs() is only used inside
>>>> "#ifdef WIN32" but was defined outside causing a
>>>> "defined but not used" warning.
>>>>
>>>> Some notes:
>>>> - Instead of using 'CSTR func = "funcname";' everywhere,
>>>> it would be better to use the __FUNCTION__ pre-processor
>>>> macro. This way, the "unused variable 'func'" is eliminated
>>>> once and for all as __FUNCTION__ is available everywhere.
>>>> - The sea of "differ in signedness" warnings are caused by
>>>> the difference of "{SQL|U}CHAR *" and plain "char *".
>>>> Many ODBC API calls expect "SQLCHAR *" input.
>>>> Using strcpy(), strcmp(), etc. on them causes warnings.
>>>> Many internal psqlODBC functions expect a character input
>>>> and they are also used inconsistenly with different
>>>> signed and unsigned parameters.
>>>> Either the API parameters has to be treated internally
>>>> as "char *" and keep a local variable for this purpose,
>>>> or pass the SQLCHAR * down in other functions which
>>>> have to be declared with the appropriate header.
>>>> Fixing it either way would be quite invasive in terms
>>>> of patch size. The latter would mean smaller and
>>>> cleaner C source.
>>>>
>>>> Best regards,
>>>> Zoltán Böszörményi
>>>>
>>>> Hiroshi Saito írta:
>>>>
>>>>
>>>>> Hi.
>>>>>
>>>>> Surely, we have not made offer sufficient about x86_64.... However,
>>>>> The check of operation
>>>>> was performed by the temporary rental machine. Moreover, there is
>>>>> also
>>>>> a situation of taking
>>>>> time in the check of the present BIGENDIAN. Then, late work may be
>>>>> blamed....
>>>>> Anyway, In order to avoid a problem, to be warning should clear.
>>>>>
>>>>> BTW, FreeBSD x86 is this.
>>>>> inet% gmake socket.o
>>>>> if gcc -DHAVE_CONFIG_H -I. -I. -I. -I/usr/local/include
>>>>> -I/usr/local/pgsql/include -Wall -g -O2 -MT socket.o -MD -MP -MF
>>>>> ".deps/socket.Tpo" -c -o socket.o socket.c; \
>>>>> then mv -f ".deps/socket.Tpo" ".deps/socket.Po"; else rm -f
>>>>> ".deps/socket.Tpo"; exit 1; fi
>>>>> socket.c: In function `SOCK_wait_for_ready':
>>>>> socket.c:468: warning: 'no_timeout' might be used uninitialized in
>>>>> this function
>>>>>
>>>>> Regards,
>>>>> Hiroshi Saito
>>>>>
>>>>> ----- Original Message ----- From: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> I wrote:
>>>>>>
>>>>>>
>>>>>>> BTW, isn't anyone paying attention to compiler warnings in this
>>>>>>> code
>>>>>>> base?
>>>>>>>
>>>>>>>
>>>>>> To be concrete, attached is a list of warnings I see when
>>>>>> building .0200
>>>>>> using gcc -Wall on an x86_64 Fedora 9 machine. If I were in
>>>>>> charge of
>>>>>> this project I'd insist on every one of these being cleaned up
>>>>>> --- they
>>>>>> are at least evidence of sloppy programming, and a significant
>>>>>> fraction
>>>>>> look like they mean certain crashes if the code ever gets executed.
>>>>>>
>>>>>> BTW, I've omitted 261 "pointer targets differ in signedness"
>>>>>> warnings...
>>>>>> those are probably not interesting, but I'd still recommend cleaning
>>>>>> them up, if only so that more important warnings don't get lost
>>>>>> in the
>>>>>> noise. The core Postgres project has maintained a zero-tolerance
>>>>>> policy
>>>>>> on gcc warnings for years, and I think it's served us well.
>>>>>>
>>>>>> regards, tom lane
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>>
>>>
>>
>>
>> --
>> ----------------------------------
>> Zoltán Böszörményi
>> Cybertec Schönig & Schönig GmbH
>> http://www.postgresql.at/
>>
>>
>
>
> --------------------------------------------------------------------------------
>
>
>
>> diff -durpN psqlodbc-08.03.0300.new1/Makefile.am
>> psqlodbc-08.03.0300.new2/Makefile.am
>> --- psqlodbc-08.03.0300.new1/Makefile.am 2008-09-30
>> 13:14:09.000000000 +0200
>> +++ psqlodbc-08.03.0300.new2/Makefile.am 2008-09-30
>> 13:36:31.000000000 +0200
>> @@ -15,8 +15,8 @@ lib_LTLIBRARIES = psqlodbcw.la
>> else
>> lib_LTLIBRARIES = psqlodbca.la
>> endif
>> -
>> -AM_CFLAGS = -Wall
>> +
>> +AM_CFLAGS = -Wall -D_BSD_SOURCE -D_XOPEN_SOURCE=500
>>
>> AM_LDFLAGS = -module -no-undefined -avoid-version
>>
>>
>
>

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/