Closed jkoshy closed 3 years ago
Seems reasonable to me, but can you add in your commit message an explanation of what's going on?
What program are you linting? (ideally it would be one of the test cases from tests/)
With what exact command line?
With what exact output?
And what is the output after this patch? is it clean now?
I have added some more detail to the commit message.
The program being linted is the source code for the NetBSD operating system.
A recent upgrade of uthash.h
in the NetBSD source tree broke its 'lint' build: https://releng.netbsd.org/builds/HEAD-lint/202011290800Z/amd64.build.failed.
The issue is that lint(1)
needs the <stdint.h>
header, but the preprocessor logic in uthash.h
currently does not cause this header to be included. The proposed change address this issue.
The change in this pull request has been folded in upstream (http://cvsweb.netbsd.org/bsdweb.cgi/src/external/bsd/elftoolchain/dist/common/uthash.h.diff?r1=1.3&r2=1.4&f=h ).
With this change NetBSD can be built from source again:
% cat /etc/mk.conf
MKLINT=yes # This knob turns on linting.
% cd $NETBSDSRC
% ./build.sh -M ~/obj -U -m i386 -u -j3 build
... lots of output ...
Successful make build
build.sh ended: Tue Dec 1 12:12:06 GMT 2020
===> .
I've revised the commit log again, to hopefully be more cogent (e70c432).
You seem to have added detail to the GitHub PR, but not to the actual commit message? 😛 Anyway, it looks like the problem with the NetBSD build is this part:
Lint pass2:
dependall ===> lib/../external/bsd/elftoolchain/lib/libdwarf
/home/source/ab/HEAD-lint/src/external/bsd/elftoolchain/lib/libdwarf/../../dist/common/uthash.h(80): typedef redeclared: uint32_t [89]
/home/builds/ab/HEAD-lint/amd64/202011290800Z-dest/usr/include/sys/stdint.h(64): previous definition of uint32_t [261]
/home/source/ab/HEAD-lint/src/external/bsd/elftoolchain/lib/libdwarf/../../dist/common/uthash.h(81): typedef redeclared: uint8_t [89]
This isn't a Lint warning; this is a compiler error! I think this error would happen literally any time somebody tried to compile a program like
#include <stdint.h>
#include <uthash.h>
int main() {}
on a compiler that (1) defined _WIN32
and/or did not define __GNUC__
, and (2) was compiling in a mode earlier than C11 mode, and (3) did not have the special-case logic that Clang apparently has to deal with this exact situation.
So now I'm less inclined to accept this patch as-is, but I agree that there's a problem here. Since <stdint.h>
has been around for 20 years now, I think the fix probably ought to be that we just unconditionally #include
it — and then, if anyone complains, we can introduce a macro
#ifndef HASH_NO_STDINT
#include <stdint.h>
#endif
and tell people to compile with -DHASH_NO_STDINT
(and manually typedef unsigned long uint32_t
or whatever) if they really need to.
Which reminds me that I have been meaning to cut a new release of uthash "in December" for a while now, and now it's December. So I should probably make this change and cut a new release, Soon. :)
You seem to have added detail to the GitHub PR, but not to the actual commit message?
Yes, that was deliberate.
It was not clear to me that URLs in a commit message would continue to be valid a few decades from now. It seemed a better idea to keep the commit and its message self-contained.
This isn't a Lint warning; this is a compiler error!
Yes, this issue isn't about the output of lint(1)
.
The issue is that compilation of uthash.h
will fail when BSD lint(1)
is used as a "compiler". lint(1)
implements a different compilation environment—one that is not completely captured by the current pre-processor logic in this file.
The #elif defined(__lint__)
stanza proposed by this patch would be parallel to the existing #if defined(_WIN32)
and #elif defined(__GNUC__)
blocks:
#if defined(_WIN32)
...
#elif defined(__GNUC__) && !defined(__VXWORKS__)
...
#elif defined(__lint__)
... New stanza here.
#else
...
#endif
I think this error would happen literally any time somebody tried to compile a program like
#include <stdint.h> #include <uthash.h> int main() {}
... Since
<stdint.h>
has been around for 20 years now, I think the fix probably ought to be that we just unconditionally#include
it — and then, if anyone complains, we can introduce a macro ...#ifndef HASH_NO_STDINT #include <stdint.h> #endif
If you are considering taking this route, you might want to consider something like:
#if __STDC_VERSION__ >= 199901L
/* C99 or later. */
#include <stdint.h>
#else
... set up types for other/older C dialects ...
#endif
@jkoshy : Please take a look at #212 — it would be especially useful to know if that PR is sufficient to fix this issue.
#if __STDC_VERSION__ >= 199901L /* C99 or later. */ #include <stdint.h> #else ... set up types for other/older C dialects ... #endif
But that's exactly the kind of code I'd like to get away from, if possible! I suppose that would be a relatively simple tweak to the existing code— i.e. we could just do
#if __STDC_VERSION__ >= 199901L || __cplusplus >= 201103L
#include <stdint.h>
#else
/* a number of the hash function use uint32_t which isn't defined on Pre VS2010 */
#if defined(_WIN32)
#if defined(_MSC_VER) && _MSC_VER >= 1600
#include <stdint.h>
#elif defined(__WATCOMC__) || defined(__MINGW32__) || defined(__CYGWIN__)
#include <stdint.h>
#else
typedef unsigned int uint32_t;
typedef unsigned char uint8_t;
#endif
#elif defined(__GNUC__) && !defined(__VXWORKS__)
#include <stdint.h>
#else
typedef unsigned int uint32_t;
typedef unsigned char uint8_t;
#endif
#endif
but I'm looking to go simpler than that.
Should be fixed by 15ad04278f19adefad34353143d944cb0c007fe2 — please let me know if it's not fixed!
Change the preprocessing logic in uthash.h to include the C99 header when the file
is being analyzed by lint(1).