Full_Name: Leonid Yuriev Version: git-master OS: RHEL7 URL: ftp://ftp.openldap.org/incoming/ Submission from: (NULL) (31.130.36.33) It is about 5K warnings reported by gcc/clang if building OpenLDAP with -Wall. CFLAGS="-Wall" ./configure ... make depend && make The vast majority of warnings about harmless things: unnecessary arguments to printf through Debug macro, unused variables and functions, etc. But such amount of warnings makes impossible to use -Wall to find a potential problems. I have prepared the patchset which fixes most of these warnings. It will be submited shortly. Leonid.
On 06/01/2015 15:47, leo@yuriev.ru wrote: > Full_Name: Leonid Yuriev > Version: git-master > OS: RHEL7 > URL: ftp://ftp.openldap.org/incoming/ > Submission from: (NULL) (31.130.36.33) > > > It is about 5K warnings reported by gcc/clang if building OpenLDAP with -Wall. > > CFLAGS="-Wall" ./configure ... > make depend && make > > The vast majority of warnings about harmless things: unnecessary arguments to > printf through Debug macro, unused variables and functions, etc. > But such amount of warnings makes impossible to use -Wall to find a potential > problems. > > I have prepared the patchset which fixes most of these warnings. > It will be submited shortly. My 2c: - normal users do not need to build with -Wall; only developers might need to - in addition to -Wall, if you add -Wno-[specific harmless warnings you want to disable], then only relevant ones do surface. p. -- Pierangelo Masarati Associate Professor Dipartimento di Scienze e Tecnologie Aerospaziali Politecnico di Milano
leo@yuriev.ru wrote: > Full_Name: Leonid Yuriev > Version: git-master > OS: RHEL7 > URL: ftp://ftp.openldap.org/incoming/ > Submission from: (NULL) (31.130.36.33) > > > It is about 5K warnings reported by gcc/clang if building OpenLDAP with -Wall. > > CFLAGS="-Wall" ./configure ... > make depend && make > > The vast majority of warnings about harmless things: unnecessary arguments to > printf through Debug macro, Ignore those. Use -Wno-format-extra-args > unused variables and functions, etc. > But such amount of warnings makes impossible to use -Wall to find a potential > problems. > > I have prepared the patchset which fixes most of these warnings. > It will be submited shortly. > > Leonid. > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
leo@yuriev.ru wrote: > Full_Name: Leonid Yuriev > Version: git-master > OS: RHEL7 > URL: ftp://ftp.openldap.org/incoming/ > Submission from: (NULL) (31.130.36.33) > > > It is about 5K warnings reported by gcc/clang if building OpenLDAP with -Wall. > > CFLAGS="-Wall" ./configure ... No need for that. Learn how to use make. > make depend && make make AC_CFLAGS="-Wall -Wno-format-extra-args" -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Yes of course, all warnings could be hushed by compiler options, but this also hides the real mistakes. So, I prefer fix the warnings, instead of disable gcc/clang features. Please review attached patchset and merge. Leonid. --- The attached files is derived from OpenLDAP Software. All of the modifications to OpenLDAP Software represented in the following patch(es) were developed by Peter-Service LLC, Moscow, Russia. Peter-Service LLC has not assigned rights and/or interest in this work to any party. I, Leonid Yuriev am authorized by Peter-Service LLC, my employer, to release this work under the following terms. Peter-Service LLC hereby places the following modifications to OpenLDAP Software (and only these modifications) into the public domain. Hence, these modifications may be freely used and/or redistributed for any purpose with or without attribution and/or other notice
Fast-forward 'pull' is also available https://github.com/leo-yuriev/openldap-lmdb-challenge.git Leonid. 2015-01-06 18:10 GMT+03:00 Leonid Yuriev <leo@yuriev.ru>: > Yes of course, all warnings could be hushed by compiler options, but this > also hides the real mistakes. > So, I prefer fix the warnings, instead of disable gcc/clang features. > > Please review attached patchset and merge. > > Leonid. > > --- > > The attached files is derived from OpenLDAP Software. All of the > modifications > to OpenLDAP Software represented in the following patch(es) were developed > by > Peter-Service LLC, Moscow, Russia. Peter-Service LLC has not assigned > rights > and/or interest in this work to any party. I, Leonid Yuriev am authorized > by > Peter-Service LLC, my employer, to release this work under the following > terms. > > Peter-Service LLC hereby places the following modifications to OpenLDAP > Software > (and only these modifications) into the public domain. Hence, these > modifications may be freely used and/or redistributed for any purpose > with or without attribution and/or other notice > > >
Leonid Yuriev wrote: > Yes of course, all warnings could be hushed by compiler options, but > this also hides the real mistakes. > So, I prefer fix the warnings, instead of disable gcc/clang features. > > Please review attached patchset and merge. Your patch assumes CPP features that are not guaranteed to be present. Rejecting this patchset. Learn more about how OpenLDAP software is built and why. > > Leonid. > > --- > > The attached files is derived from OpenLDAP Software. All of the > modifications > to OpenLDAP Software represented in the following patch(es) were > developed by > Peter-Service LLC, Moscow, Russia. Peter-Service LLC has not assigned > rights > and/or interest in this work to any party. I, Leonid Yuriev am > authorized by > Peter-Service LLC, my employer, to release this work under the following > terms. > > Peter-Service LLC hereby places the following modifications to OpenLDAP > Software > (and only these modifications) into the public domain. Hence, these > modifications may be freely used and/or redistributed for any purpose > with or without attribution and/or other notice > > -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
On 01/06/2015 04:31 PM, hyc@symas.com wrote: > Leonid Yuriev wrote: >> Yes of course, all warnings could be hushed by compiler options, but >> this also hides the real mistakes. >> So, I prefer fix the warnings, instead of disable gcc/clang features. >> >> Please review attached patchset and merge. > > Your patch assumes CPP features that are not guaranteed to be present. > Rejecting this patchset. The Debug() change is going to give merge conflicts all over the place when your <https://github.com/ReOpen/ReOpenLDAP> and OpenLDAP pull updates from each other. How about defining Debug1(), Debug2(), etc for 1,2... arguments? That's still annoying, but it keeps the code C90-compatible and keeps -Wformat quiet. There's already similar code for Log<N>() in include/ldap_log.h, but it was never used much or even merged into RE24. (Because it got scheduled for RE25 which was going to start Real Soon Now, IIRC:-) -- Hallvard
2015-01-12 17:19 GMT+03:00 Hallvard Breien Furuseth < h.b.furuseth@usit.uio.no>: > On 01/06/2015 04:31 PM, hyc@symas.com wrote: > >> Leonid Yuriev wrote: >> >>> Yes of course, all warnings could be hushed by compiler options, but >>> this also hides the real mistakes. >>> So, I prefer fix the warnings, instead of disable gcc/clang features. >>> >>> Please review attached patchset and merge. >>> >> >> Your patch assumes CPP features that are not guaranteed to be present. >> Rejecting this patchset. >> > > The Debug() change is going to give merge conflicts all over > the place when your <https://github.com/ReOpen/ReOpenLDAP> and > OpenLDAP pull updates from each other. > > https://github.com/ReOpen/ReOpenLDAP should not be used for merge, but only for cherry-picking. It has stripped history and merged with OpenLDAP/mdb.master, because mdb.RE/0.9 don't includes a few critical fixes (ITS 7969,7970,7971) for our case. I think that is the reason for patch conflicts. Patchset got RE25 should remain among my sent emails. Presumably I can send it for you again, if you are interested. > How about defining Debug1(), Debug2(), etc for 1,2... arguments? > That's still annoying, but it keeps the code C90-compatible and > keeps -Wformat quiet. There's already similar code for Log<N>() > in include/ldap_log.h, but it was never used much or even merged > into RE24. (Because it got scheduled for RE25 which was going to > start Real Soon Now, IIRC:-) > > -- > Hallvard > IMHO the Log<N> recipe is onerously for support. On other hand, nowadays all non-obsolete compilers support most of C99 features. Baseline features, such as "variadic macro", supported long ago (Microsoft was the most conservative to 2012). Maybe it is time to enable C99 in RE25? Leonid.
On 01/12/2015 05:22 PM, leo@yuriev.ru wrote: > 2015-01-12 17:19 GMT+03:00 Hallvard Breien Furuseth < > h.b.furuseth@usit.uio.no>: >> The Debug() change is going to give merge conflicts all over >> the place when your <https://github.com/ReOpen/ReOpenLDAP> and >> OpenLDAP pull updates from each other. >> >> https://github.com/ReOpen/ReOpenLDAP should not be used for merge, but > only for cherry-picking. Oops, right. "cherry-picking conflicts":-) > It has stripped history and merged with OpenLDAP/mdb.master, because > mdb.RE/0.9 don't includes a few critical fixes (ITS 7969,7970,7971) for our > case. > I think that is the reason for patch conflicts. > Patchset got RE25 should remain among my sent emails. Presumably I can send > it for you again, if you are interested. No, I was talking about maintaining the two projects in the future. Presumably you'll want to copy fixes from OpenLDAP, and we'll want fixes from you. Any such change next to a Debug() statement will not apply cleanly, and must be hand-edited. >> How about defining Debug1(), Debug2(), etc for 1,2... arguments? >> That's still annoying, but it keeps the code C90-compatible and >> keeps -Wformat quiet. There's already similar code for Log<N>() >> in include/ldap_log.h, but it was never used much or even merged >> into RE24. (Because it got scheduled for RE25 which was going to >> start Real Soon Now, IIRC:-) > > IMHO the Log<N> recipe is onerously for support. > On other hand, nowadays all non-obsolete compilers support most of C99 > features. > Baseline features, such as "variadic macro", supported long ago (Microsoft > was the most conservative to 2012). > Maybe it is time to enable C99 in RE25? Dunno. People do use old compilers. OpenLDAP is conservative about portability. It requires C90 to build, but exports K&R1-compatible headers. Well, it tries. K&R1 compat was broken by OpenLDAP 2.0 (ITS#6171) and nobody who cares about K&R1 has bothered to fix it. When we do move towards C99, maybe we'll first test the waters by making Debug() use __VA_ARGS__ but not change the Debug() calls yet, so we'll only have a few lines to revert it it proves premature. We'll hear what Kurt says. -- Hallvard
I unexpectedly detected that the variadic macro feature of C99 was already used in OpenLDAP 2.4 ;) grep -R __VA_ARGS__ * build/ltmain.sh:# define DEBUG(format, ...) fprintf(stderr, format, __VA_ARGS__) contrib/ldapc++/ltmain.sh:# define DEBUG(format, ...) fprintf(stderr, format, __VA_ARGS__) libraries/liblmdb/mdb.c: fprintf(stderr, "%s:%d " fmt "\n", mdb_func_, __LINE__, __VA_ARGS__) servers/slapd/back-bdb/back-bdb.h:#define BDB_LOG_PRINTF(env,txn,fmt,...) (env)->log_printf((env),(txn),(fmt),__VA_ARGS__) servers/slapd/back-bdb/back-bdb.h:#define BDB_LOG_PRINTF(env,txn,fmt,...) __db_logmsg((env),(txn),"DIAGNOSTIC",0,(fmt),__VA_ARGS__)
On 22/01/15 06:13, leo@yuriev.ru wrote: > I unexpectedly detected that the variadic macro feature of C99 was > already used in OpenLDAP 2.4 ;) Look a bit closer: > grep -R __VA_ARGS__ * > build/ltmain.sh:# define DEBUG(format, ...) fprintf(stderr, format, > __VA_ARGS__) > contrib/ldapc++/ltmain.sh:# define DEBUG(format, ...) fprintf(stderr, > format, __VA_ARGS__) ltmain.sh is a shell/code library generated by GNU libtool. Lots of stuff in there which we do not use. > libraries/liblmdb/mdb.c: fprintf(stderr, "%s:%d " fmt "\n", > mdb_func_, __LINE__, __VA_ARGS__) > servers/slapd/back-bdb/back-bdb.h:#define > BDB_LOG_PRINTF(env,txn,fmt,...) > (env)->log_printf((env),(txn),(fmt),__VA_ARGS__) > servers/slapd/back-bdb/back-bdb.h:#define > BDB_LOG_PRINTF(env,txn,fmt,...) > __db_logmsg((env),(txn),"DIAGNOSTIC",0,(fmt),__VA_ARGS__) Debug macros which are off by default and fall back to ((void)0) or somesuch.
> * > Date: Thu, 22 Jan 2015 07:49:15 +0100 > From: Hallvard Breien Furuseth <h.b.furuseth@usit.uio.no> > To: leo@yuriev.ru, openldap-its@OpenLDAP.org > Subject: Re: (ITS#8018) a lot of warnings building with -Wall > * > On 22/01/15 06:13, leo@yuriev.ru wrote: > > I unexpectedly detected that the variadic macro feature of C99 was > > already used in OpenLDAP 2.4 ;) > > Look a bit closer: > > > grep -R __VA_ARGS__ * > > build/ltmain.sh:# define DEBUG(format, ...) fprintf(stderr, format, > > __VA_ARGS__) > > contrib/ldapc++/ltmain.sh:# define DEBUG(format, ...) fprintf(stderr, > > format, __VA_ARGS__) > > ltmain.sh is a shell/code library generated by GNU libtool. > Lots of stuff in there which we do not use. > > > libraries/liblmdb/mdb.c: fprintf(stderr, "%s:%d " fmt "\n", > > mdb_func_, __LINE__, __VA_ARGS__) > > servers/slapd/back-bdb/back-bdb.h:#define > > BDB_LOG_PRINTF(env,txn,fmt,...) > > (env)->log_printf((env),(txn),(fmt),__VA_ARGS__) > > servers/slapd/back-bdb/back-bdb.h:#define > > BDB_LOG_PRINTF(env,txn,fmt,...) > > __db_logmsg((env),(txn),"DIAGNOSTIC",0,(fmt),__VA_ARGS__) > > Debug macros which are off by default and fall back to ((void)0) > or somesuch. Hallvard, thanks. Of course, I understand that __VA_ARGS__ are used only for debugging currently. But this is makes me think that recent development and testing of OpenLDAP are carried out only in today's environment (modern Linux and gcc). Could anyone say about the list of a system-compiler pairs with which OpenLDAP was tested or just successfully compiled? For instance, in our fork we plan limit the support to modern systems with gcc, clang and last msvc compilers. However, users ask us - What platforms are not supported by your fork in comparison with original OpenLDAP? For example https://gcc.gnu.org/gcc-4.1/buildstat.html Is there something similar for OpenLDAP? Leonid.
On 24/01/15 07:26, leo@yuriev.ru wrote: > Of course, I understand that __VA_ARGS__ are used only for debugging > currently. Aha, a misunderstanding there. Debug() is _not_ just for debugging, it's also for logging. "Rebus" coding, as you say:-( 20 years of cruft in the code. I see you've talked about that in ITS#8015, so I'll answer the rest there instead. > But this is makes me think that recent development and testing of > OpenLDAP are carried out only in today's environment (modern Linux and gcc). Certainily not. _Mainly_ on a few platforms, probably. People submit bug reports about other platforms, Howard once in a while says "I tested on <that platform> and did/did not find your problem", etc. See the commit logs and mailinglists, and the Build Environment part of CHANGES in REL_ENG: gcc-isms and other stuff creep in, and get thrown out again. > Could anyone say about the list of a system-compiler pairs with which > OpenLDAP was tested or just successfully compiled? Not to my knowledge. We just try to keep the code portable, within some constraints like we don't support weirdness like 32-bit 'char'. > For instance, in our fork we plan limit the support to modern systems > with gcc, clang and last msvc compilers. That's incompatible with our coding practice. We just try to keep the code portable, within some constraints like we don't support weirdness like 32-bit 'char' and some religious disagreements about what portability is. Anyway, if some code isn't portable, it needs a good excuse for getting in and staying in. Note that lots of unportable code can be writtene portably, or at least it can keep the unportability in one place. E.g. your repo's change of /bin/sh to bash is not an option for us. But maybe tests/run.in and tests/scripts/all could be changed to optionally invoke them with another shell. -- Hallvard