Diff Detail
Event Timeline
| lib/asan/tests/CMakeLists.txt | ||
|---|---|---|
| 38 ↗ | (On Diff #7708) | This comment could use more clarity; presumably you meant to add "in 32-bit mode"? |
Can we instead #define _SIZE_T_DECLARED in source files? Maybe we can add this to lib/sanitizer_common/tests/sanitizer_test_utils.h
That would require several headers to be altered or otherwise the headers in the unittests sources will be forced to come in a specific order. Neither of the ways is better than a single change in the CMakeLists.txt, I guess?
asan_linux.cc changes moved to a separate diff:
http://llvm-reviews.chandlerc.com/D3039
What error do you observe if _SIZE_T_DECLARED is not defined? How does other programs on FreeBSD work around these issue? I'm not convinced that introducing a hack to the build
system is appropriate here. It will also unlikely be the only place where we use it - e.g. would you need the same for building sanitizer_common unit tests (run by check-sanitizer command)?
What error do you observe if _SIZE_T_DECLARED is not defined? How does other programs on FreeBSD work around these issue?
Without _SIZE_T_DECLARED we get redefinition errors for size_t--unsigned int vs. unsigned long or vice verse, depending on whether size_t is first defined in a system header or in sanitizers' sources.
It will also unlikely be the only place where we use it - e.g. would you need the same for building sanitizer_common unit tests (run by check-sanitizer command)?
Yes, most likely we will need to fix that it a couple more places.
Please let me know if you have any suggestions. Thanks.
Where is size_t defined in sanitizer sources? Do you mean that
#define size_t unsigned
magic in asan_new_delete.cc? It's not a header file, how can it affect building the unit test?
Where is size_t defined in sanitizer sources?
I probably misworded the reference to sanitizers sources. The point is that clang has its own set of headers it use in place of the system ones (see /tools/clang/lib/Headers). One of the headers--<stddef.h>, namely--define 'size_t' as:
typedef __SIZE_TYPE__ size_t;
which is the correct definition on all platforms, but conflicts with the incorrect one:
// <machine/_types.h> typedef __uint64_t __size_t; // <sys/types.h> #ifndef _SIZE_T_DECLARED typedef __size_t size_t; #define _SIZE_T_DECLARED #endif
on FreeBSD 9.2 in 32-bit mode (-m32). Depending on which of the headers--the system or the clang's one--is included first 'size_t' is defined as 'unsigned int' or 'unsigned long'. Tracking potential [indirect] inclusions of <sys/types.h> doesn't seem to be trivial so I see two alternatives: either define _SIZE_T_DECLARED in all test sources before any #includes or put -D_SIZE_T_DECLARED to the CMakeLists.txt files.
Do I understand correctly that this problem will fire on *any* program that is:
a) compiled with Clang on FreeBSD 9.2
b) includes stddef.h (i.e. Clang's header)
c) includes sys/types.h
it seems impractical to force all FreeBSD users provide -D_SIZE_T_DECLARED. Can we work around this issue in Clang's stddef.h instead? E.g. use:
#undef size_t typedef __SIZE_TYPE__ size_t; #define _SIZE_T_DECLARED
Do I understand correctly that this problem will fire on *any* program that is:
a) compiled with Clang on FreeBSD 9.2
b) includes stddef.h (i.e. Clang's header)
c) includes sys/types.h
Yes, any program that is a), b) and includes one of the numerous system headers that directly define 'size_t' (and thus directly or indirectly include <sys/types.h>).
it seems impractical to force all FreeBSD users provide -D_SIZE_T_DECLARED.
Well, bugs are usually a kind of impractical thing? :-)
Can we work around this issue in Clang's stddef.h instead? E.g. use:
#undef size_t
typedef SIZE_TYPE size_t;
#define _SIZE_T_DECLARED
No, since we can't be sure <stddef.h> is included in front of other system headers.
Why do we use
#define size_t unsigned
in lib/asan/asan_new_delete.cc instead of the fix you suggest in this patch?
What if we (say) use #define size_t unsigned for old FreeBSD in Clang's stddef.h. Would that work? Or we will have problems with functions defined between the inclusion of (broken) system headers and the inclusion of stddef.h?
Why do we use
#define size_t unsigned
in lib/asan/asan_new_delete.cc instead of the fix you suggest in this patch?
If we define _SIZE_T_DECLARED for this set of sources, then we need to define size_t in each of the sources which is a kind of mass change--no good. And then we would need both the CMakeLists.txt and the sources, so what's the profit?
Note that inclusion of <stddef.h> doesn't help as it doesn't actually refer to the clang's one when the run-time lib get compiled first time, that is, with a host compiler.
What if we (say) use #define size_t unsigned for old FreeBSD in Clang's stddef.h. Would that work? Or we will have problems with functions defined between the inclusion of (broken) system headers and the inclusion of stddef.h?
Again, it wouldn't work as we cannot rely on that the host compiler is a specific version of clang.
Furthermore, pre-installed clang on FreeBSD is not exactly one could get with (cmake && gmake && gmake install) as FreeBSD's ports--including clang--does not use cmake and build and install from plain makefiles. The latters sometimes differ from the corresponding version of clang is expected to be. This would server as an example of such a deviation:
If we define _SIZE_T_DECLARED for this set of sources, then we need to define size_t in each of the sources which is a kind of mass change--no good. And then we would need both the CMakeLists.txt and the sources, so what's the profit?
What if we compile all the sources and all the tests with "_D_SIZE_T_DECLARED" and "-Dsize_t=unsigned" (or -Dsize_t=SIZE_TYPE)?
What if we compile all the sources and all the tests with "_D_SIZE_T_DECLARED" and "-Dsize_t=unsigned" (or -Dsize_t=SIZE_TYPE)?
It fails on:
// bits/c++config.h
namespace std
{
typedef __SIZE_TYPE__ size_t;with this:
/usr/include/c++/4.8.1/x86_64-unknown-freebsd9.2/bits/c++config.h:180:26: error: cannot combine with previous 'int' declaration specifier
typedef __SIZE_TYPE__ size_t;
^
<command line>:2:16: note: expanded from here
#define size_t __SIZE_TYPE__
^
<built-in>:63:37: note: expanded from here
#define __SIZE_TYPE__ long unsigned int
^There are no macro guards for the 'size_t' definition.
Alright, does it make sense to compile all the compiler-rt code and tests (not only files under lib/asan/tests) with -D_SIZE_T_DECLARED?
Alright, does it make sense to compile all the compiler-rt code and tests (not only files under lib/asan/tests) with -D_SIZE_T_DECLARED?
Re: compiler-rt code: defining _SIZE_T_DECLARED will lead to the undefined size_t kind of errors. Once again, we should keep in mind that not all of the sources include <stddef.h> in front of other headers and the sources get first built with a host compiler, which is not necessarily a version of clang.
Re: building tests: yes, adding -D_SIZE_T_DECLARED to COMPILER_RT_GTEST_INCLUDE_CFLAGS (set in cmake/Modules/AddCompilerRT.cmake) does the work.
Re: building tests: yes, adding -D_SIZE_T_DECLARED to COMPILER_RT_GTEST_INCLUDE_CFLAGS (set in cmake/Modules/AddCompilerRT.cmake) does the work.
Let's do it that way then. I've renamed this var to COMPILER_RT_GTEST_CFLAGS. We can add -D_SIZE_T_DECLARED to it on FreeBSD 9.2.
| cmake/Modules/AddCompilerRT.cmake | ||
|---|---|---|
| 117 | I believe CMAKE_SIZEOF_VOID_P MATCHES "8" means 64-bit host OS so it doesn't relate to the building mode. | |
| cmake/Modules/AddCompilerRT.cmake | ||
|---|---|---|
| 118 | The same issue exists in all older versions, too. I've merged (most of) the header changes to support -m32 to FreeBSD stable/9, so this should work properly in FreeBSD 9.3. | |
D3313 replaces this diff.
The same issue exists in all older versions, too.
OK, I will note that in further changes.
I've merged (most of) the header changes to support -m32 to FreeBSD stable/9, so this should work properly in FreeBSD 9.3.
Thanks, Ed!
I'm confused: why do you mention 32-bit mode here and use "sizeof(void*) == 8" below? Please change the wording of the comment.