Bug 454030 - Need to address 64-bit compiler warnings
Need to address 64-bit compiler warnings
Status: CLOSED CURRENTRELEASE
Product: 389
Classification: Community
Component: Directory Server (Show other bugs)
1.1.1
All Linux
low Severity low
: ---
: ---
Assigned To: Rich Megginson
Chandrasekar Kannan
:
Depends On:
Blocks: 249650 FDS1.2.0
  Show dependency treegraph
 
Reported: 2008-07-03 18:02 EDT by Nathan Kinder
Modified: 2015-01-04 18:33 EST (History)
4 users (show)

See Also:
Fixed In Version: 8.1
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-29 19:04:50 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Compiler Warnings (28.45 KB, text/plain)
2008-07-03 18:02 EDT, Nathan Kinder
no flags Details
diffs - part 1 (179.82 KB, patch)
2008-10-01 10:22 EDT, Rich Megginson
no flags Details | Diff
fixed dna.c diffs (14.55 KB, patch)
2008-10-03 18:49 EDT, Rich Megginson
no flags Details | Diff
cvs commit log - part 1 (14.41 KB, text/plain)
2008-10-08 13:37 EDT, Rich Megginson
no flags Details
cvs commit log - part 1 - for bug 463991 too (801 bytes, text/plain)
2008-10-09 13:41 EDT, Rich Megginson
no flags Details
cvs commit log (1.70 KB, text/plain)
2008-10-17 12:55 EDT, Rich Megginson
no flags Details
more fixes (281.04 KB, patch)
2008-12-04 22:56 EST, Rich Megginson
no flags Details | Diff
cvs commit log - more fixes (13.61 KB, text/plain)
2008-12-05 17:52 EST, Rich Megginson
no flags Details

  None (edit)
Description Nathan Kinder 2008-07-03 18:02:09 EDT
We have a number of places in the code that perform casts between "void *" and
"int".  On a 64-bit machine, these are not the same size, which could cause
problems.  We should address these warnings.
Comment 1 Nathan Kinder 2008-07-03 18:02:09 EDT
Created attachment 310967 [details]
Compiler Warnings
Comment 2 Noriko Hosoi 2008-07-03 19:32:46 EDT
notes on ldbm_config:
convert the following "types" to pointer (void *) in ldbm_config_set
    integer, octal, long, size_t, string, onoff
and they are converted back to each type in each config set function.
Indeed, the size of integer (4) and the size of pointer (8) do not match on the
64-bit machine, but integer->pointer->integer is supposed to be guaranteed.
Comment 3 Rich Megginson 2008-10-01 10:22:57 EDT
Created attachment 318201 [details]
diffs - part 1
Comment 4 Rich Megginson 2008-10-03 18:49:54 EDT
Created attachment 319432 [details]
fixed dna.c diffs
Comment 5 Rich Megginson 2008-10-03 18:50:57 EDT
Comment on attachment 319432 [details]
fixed dna.c diffs

also merged with Nathan's latest commit
Comment 6 Rich Megginson 2008-10-08 13:37:57 EDT
Created attachment 319773 [details]
cvs commit log - part 1

Reviewed by: nhosoi (Thanks!)
Fix Description: The intptr_t and uintptr_t are types which are defined as integer types that are the same size as the pointer (void *) type.  On the platforms we currently support, this is the same as long and unsigned long, respectively (ILP32 and LP64).  However, intptr_t and uintptr_t are more portable.  These can be used to assign a value passed as a void * to get an integer value, then "cast down" to an int or PRBool, and vice versa.  This seems to be a common idiom in other applications where values must be passed as void *.
For the printf/scanf formats, there is a standard header called inttypes.h which defines formats to use for various 64 bit quantities, so that you don't need to figure out if you have to use %lld or %ld for a 64-bit value - you just use PRId64 which is set to the correct value.  I also assumed that size_t is defined as the same size as a pointer so I used the PRIuPTR format macro for size_t.
I removed many unused variables and some unused functions.
I put parentheses around assignments in conditional expressions to tell the compiler not to complain about them.
I cleaned up some #defines that were defined more than once.
I commented out some unused goto labels.
Some of our header files shared among several source files define static variables.  I made it so that those variables are not defined unless a macro is set in the source file.  This avoids a lot of unused variable warnings.
I added some return values to functions that were declared as returning a value but did not return a value.  In all of these cases no one was checking the return value anyway.
I put explicit parentheses around cases like this: expr || expr && expr - the && has greater precedence than the ||.  The compiler complains because it wants you to make sure you mean expr || (expr && expr), not (expr || expr) && expr.
I cleaned up several places where the compiler was complaining about possible use of uninitialized variables.  There are still a lot of these cases remaining.
There are a lot of warnings like this:
lib/ldaputil/certmap.c:1279: warning: dereferencing type-punned pointer will break strict-aliasing rules
These are due to our use of void ** to pass in addresses of addresses of structures.  Many of these are calls to slapi_ch_free, but many are not - they are cases where we do not know what the type is going to be and may have to cast and modify the structure or pointer.  I started replacing the calls to slapi_ch_free with slapi_ch_free_string, but there are many many more that need to be fixed.
The dblayer code also contains a fix for https://bugzilla.redhat.com/show_bug.cgi?id=463991 - instead of checking for dbenv->foo_handle to see if a db "feature" is enabled, instead check the flags passed to open the dbenv.  This works for bdb 4.2 through bdb 4.7 and probably other releases as well.
Platforms tested: RHEL5 x86_64, Fedora 8 i386
Flag Day: no
Doc impact: no
Comment 7 Rich Megginson 2008-10-09 10:58:01 EDT
Missed one from my previous commit:
Checking in ldif2ldbm.c;
/cvs/dirsec/ldapserver/ldap/servers/slapd/back-ldbm/ldif2ldbm.c,v  <--  ldif2ldbm.c
new revision: 1.18; previous revision: 1.17
done
Comment 8 Rich Megginson 2008-10-09 13:41:09 EDT
Created attachment 319885 [details]
cvs commit log - part 1 - for bug 463991 too

Fix Description: I inadvertantly committed fixes for 463991 along with fixes for 454030, and the fixes for 463991 broke the server.  This commit makes the server work again.
Comment 9 Rich Megginson 2008-10-17 12:55:39 EDT
Created attachment 320698 [details]
cvs commit log

Fix Description: As it turns out, there is no portable format specifier
for size_t that works on all of our supported platforms.  Afaict, %lu should
work everywhere.  C99 uses the "z" specifier, but alas not all of the compilers
we use support C99 and/or "z".
Platforms tested: RHEL5, Solaris
Flag Day: no
Doc impact: no
Comment 10 Rich Megginson 2008-12-04 22:56:01 EST
Created attachment 325774 [details]
more fixes
Comment 11 Rich Megginson 2008-12-05 17:52:49 EST
Created attachment 325921 [details]
cvs commit log - more fixes

Reviewed by: nhosoi (Thanks!)
Fix Description: This patch cleans up most of the other remaining compiler warnings.  I compiled the directory server code with these flags on RHEL5 x86_64: -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic
I also enabled argument/format match checking for most of the commonly used varadic functions.  Most of the problems I found fell into these categories:
1) Too many or not enough arguments e.g. most everything that uses or did use LDAPDebug had extra 0,0 arguments.  If they had been switched to use slapi_log_error, I removed the extra arguments - for those places still using LDAPDebug, I introduced more macros to handle the number of arguments, since C macros cannot be varadic.
2) When using NSPR formatting functions, we have to use %llu or %lld for 64-bit values, even on 64-bit systems.  However, for regular system formatting functions, we have to use %ld or %lu.  I introduced two new macros NSPRIu64 and NSPRI64 to handle cases where we are passing explicit 64-bit values to NSPR formatting functions, so that we can use the regular PRIu64 and PRI64 macros for regular system formatting functions.  I also made sure we used NSPRI* only with NSPR functions, and used PRI* only with system functions.
3) use %lu for size_t and %ld for time_t
I did find a few "real" errors, places that the code was doing something definitely not right:
https://bugzilla.redhat.com/attachment.cgi?id=325774&action=diff#ldapserver/ldap/servers/plugins/acl/aclinit.c_sec4
https://bugzilla.redhat.com/attachment.cgi?id=325774&action=diff#ldapserver/ldap/servers/plugins/acl/acllas.c_sec17
https://bugzilla.redhat.com/attachment.cgi?id=325774&action=diff#ldapserver/ldap/servers/plugins/http/http_impl.c_sec1
https://bugzilla.redhat.com/attachment.cgi?id=325774&action=diff#ldapserver/ldap/servers/plugins/memberof/memberof.c_sec1
https://bugzilla.redhat.com/attachment.cgi?id=325774&action=diff#ldapserver/ldap/servers/plugins/pam_passthru/pam_ptimpl.c_sec1
https://bugzilla.redhat.com/attachment.cgi?id=325774&action=diff#ldapserver/ldap/servers/plugins/replication/cl5_api.c_sec5
https://bugzilla.redhat.com/attachment.cgi?id=325774&action=diff#ldapserver/ldap/servers/plugins/replication/cl5_clcache.c_sec2
https://bugzilla.redhat.com/attachment.cgi?id=325774&action=diff#ldapserver/ldap/servers/plugins/replication/replutil.c_sec1
https://bugzilla.redhat.com/attachment.cgi?id=325774&action=diff#ldapserver/ldap/servers/slapd/libglobs.c_sec1
https://bugzilla.redhat.com/attachment.cgi?id=325774&action=diff#ldapserver/ldap/servers/slapd/back-ldbm/dbverify.c_sec2
https://bugzilla.redhat.com/attachment.cgi?id=325774&action=diff#ldapserver/ldap/servers/slapd/back-ldbm/ldif2ldbm.c_sec3
This is why it's important to use this compiler checking, and why it's important to fix compiler warnings, if for no other reason than the sheer noise from so many warnings can mask real errors.
Platforms tested: RHEL5
Flag Day: no
Doc impact: no
Comment 12 Jenny Galipeau 2009-03-13 12:44:31 EDT
I'm not sure this is something that QE can verify.
Comment 13 Nathan Kinder 2009-03-13 12:54:07 EDT
(In reply to comment #12)
> I'm not sure this is something that QE can verify.  

Nope.  We should just close this.
Comment 14 Chandrasekar Kannan 2009-04-29 19:04:50 EDT
An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHEA-2009-0455.html

Note You need to log in before you can comment on or make changes to this bug.