Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 454030 - Need to address 64-bit compiler warnings
Summary: Need to address 64-bit compiler warnings
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: 389
Classification: Retired
Component: Directory Server
Version: 1.1.1
Hardware: All
OS: Linux
low
low
Target Milestone: ---
Assignee: Rich Megginson
QA Contact: Chandrasekar Kannan
URL:
Whiteboard:
Depends On:
Blocks: FDS1.2.0
TreeView+ depends on / blocked
 
Reported: 2008-07-03 22:02 UTC by Nathan Kinder
Modified: 2015-01-04 23:33 UTC (History)
4 users (show)

Fixed In Version: 8.1
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-04-29 23:04:50 UTC


Attachments (Terms of Use)
Compiler Warnings (deleted)
2008-07-03 22:02 UTC, Nathan Kinder
no flags Details
diffs - part 1 (deleted)
2008-10-01 14:22 UTC, Rich Megginson
no flags Details | Diff
fixed dna.c diffs (deleted)
2008-10-03 22:49 UTC, Rich Megginson
no flags Details | Diff
cvs commit log - part 1 (deleted)
2008-10-08 17:37 UTC, Rich Megginson
no flags Details
cvs commit log - part 1 - for bug 463991 too (deleted)
2008-10-09 17:41 UTC, Rich Megginson
no flags Details
cvs commit log (deleted)
2008-10-17 16:55 UTC, Rich Megginson
no flags Details
more fixes (deleted)
2008-12-05 03:56 UTC, Rich Megginson
no flags Details | Diff
cvs commit log - more fixes (deleted)
2008-12-05 22:52 UTC, Rich Megginson
no flags Details

Description Nathan Kinder 2008-07-03 22:02:09 UTC
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 22:02:09 UTC
Created attachment 310967 [details]
Compiler Warnings

Comment 2 Noriko Hosoi 2008-07-03 23:32:46 UTC
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 14:22:57 UTC
Created attachment 318201 [details]
diffs - part 1

Comment 4 Rich Megginson 2008-10-03 22:49:54 UTC
Created attachment 319432 [details]
fixed dna.c diffs

Comment 5 Rich Megginson 2008-10-03 22:50:57 UTC
Comment on attachment 319432 [details]
fixed dna.c diffs

also merged with Nathan's latest commit

Comment 6 Rich Megginson 2008-10-08 17:37:57 UTC
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 14:58:01 UTC
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 17:41:09 UTC
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 16:55:39 UTC
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-05 03:56:01 UTC
Created attachment 325774 [details]
more fixes

Comment 11 Rich Megginson 2008-12-05 22:52:49 UTC
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 16:44:31 UTC
I'm not sure this is something that QE can verify.

Comment 13 Nathan Kinder 2009-03-13 16:54:07 UTC
(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 23:04:50 UTC
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.