This is an archive of the discontinued LLVM Phabricator instance.

Do not process __config file in C mode
AbandonedPublic

Authored by aizatsky on Oct 16 2015, 1:28 PM.

Details

Summary

stddef.h (and other files) leak into C compilation while building Chrome.
Protecting __config file with _cplusplus guard.

Diff Detail

Event Timeline

aizatsky updated this revision to Diff 37633.Oct 16 2015, 1:28 PM
aizatsky retitled this revision from to guard __config.
aizatsky updated this object.
aizatsky retitled this revision from guard __config to Do not process __config file in C mode.Oct 16 2015, 1:32 PM
aizatsky updated this object.
aizatsky added reviewers: rsmith, mclow.lists, EricWF.
aizatsky added a subscriber: llvm-commits.
This comment was removed by aizatsky.

Please ignore last comment.

EricWF requested changes to this revision.Oct 16 2015, 2:11 PM
EricWF edited edge metadata.

This is not OK. First, if __config is first included within an extern "C" it will define the header guard macro but nothing else. Later, when __config is included again from a C++ file it won't provide any of its contents because the header guard is already defined.

Also all of the C headers provided by libc++ depend on <__config> to provide the _LIBCPP_HAS_NO_SYSTEM_HEADER_PRAGMA and _LIBCPP_VERSION. However this dependancy is small enough we can probably work around it in the C headers.

stddef.h (and other files) leak into C compilation while building Chrome.

Weird, libc++'s include paths shouldn't be added by the driver during C compilation AFAIK. So it's stddef.h header shouldn't be found.
Are you using the clang C driver when compiling? I know your issue can arise when using the C++ driver and compiling code in extern "C" blocks,
but then your still compiling C++ code and you want our C headers.

Also <__config> should be safe to include during C compilation, if unfortuante. Can you elaborate on the problem's this is causing?

This revision now requires changes to proceed.Oct 16 2015, 2:11 PM

This is the typical error that happens during Chrome compile:

FAILED: ../../third_party/llvm-build/Release+Asserts/bin/clang -MMD -MF obj/third_party/libxml/libxml2/dict.o.d -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2 -DENABLE_MDNS=1 -DENABLE_NOTIFICATIONS -DENABLE_PEPPER_CDMS -DENABLE_PLUGINS=1 -DENABLE_PDF=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DDONT_EMBED_BUILD_METADATA -DUSE_UDEV -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_ASH=1 -DUSE_AURA=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_CLIPBOARD_AURAX11=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_GLIB=1 -DUSE_OPENSSL=1 -DUSE_NSS_CERTS=1 -DUSE_X11=1 -DNO_TCMALLOC -DMEMORY_TOOL_REPLACES_ALLOCATOR -DMEMORY_SANITIZER_INITIAL_SIZE -DADDRESS_SANITIZER -DENABLE_WEBRTC=1 -DDISABLE_NACL -DENABLE_EXTENSIONS=1 -DENABLE_CONFIGURATION_POLICY -DENABLE_TASK_MANAGER=1 -DENABLE_THEMES=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_GOOGLE_NOW=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_HIDPI=1 -DENABLE_TOPCHROME_MD=1 -DV8_USE_EXTERNAL_STARTUP_DATA -DENABLE_BACKGROUND=1 -DENABLE_PRE_SYNC_BACKUP -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL -DCHROMIUM_BUILD -DENABLE_MEDIA_ROUTER=1 -DFIELDTRIAL_TESTING_ENABLED -DCR_CLANG_REVISION=247874-1 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_DEBUG -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -D_GLIBCXX_DEBUG=1 -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -I../../third_party/libxml/linux -I../.. -Igen -I../../buildtools/third_party/libc++/trunk/include -I../../buildtools/third_party/libc++abi/trunk/include -I../../third_party/libxml/src/include -I../../third_party/libxml/linux/include -I../../third_party/icu/source/common -I../../third_party/icu/source/i18n -I../../third_party/zlib -fno-strict-aliasing --param=ssp-buffer-size=4 -fstack-protector -funwind-tables -fPIC -pipe -B../../third_party/binutils/Linux_x64/Release/bin -fcolor-diagnostics -pthread -m64 -march=x86-64 -O0 -g2 -gsplit-dwarf -fno-omit-frame-pointer -gline-tables-only -fsanitize=address -fsanitize-blacklist=../../tools/memory/asan/blacklist.txt -fsanitize-coverage=edge,indirect-calls,8bit-counters,trace-cmp -fvisibility=hidden -Xclang -load -Xclang ../../third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so -Xclang -plugin-arg-find-bad-constructs -Xclang check-templates -Xclang -add-plugin -Xclang find-bad-constructs -Wheader-hygiene -Wstring-conversion -Wno-unused-result -Wno-unused-variable -Wno-format -Wendif-labels -Werror -Wno-missing-field-initializers -Wno-unused-parameter -Wno-c++11-narrowing -Wno-char-subscripts -Wno-covered-switch-default -Wno-deprecated-register -Wno-unneeded-internal-declaration -Wno-inconsistent-missing-override -Wno-reserved-user-defined-literal -Wno-shift-negative-value -Wno-bitfield-width -Wno-pointer-sign -Wno-empty-body -Wno-tautological-pointer-compare -Wno-ignored-attributes -Wno-int-to-void-pointer-cast -Wno-incompatible-pointer-types -Wno-unused-function -DLIBXML_STATIC= -c ../../third_party/libxml/src/dict.c -o obj/third_party/libxml/libxml2/dict.o
In file included from ../../third_party/libxml/src/dict.c:20:
In file included from ../../third_party/libxml/src/libxml.h:49:
In file included from ../../buildtools/third_party/libc++/trunk/include/stdio.h:102:
../../buildtools/third_party/libc++/trunk/include/__config:270:9: error: unknown type name 'char16_t'
typedef
char16_t char16_t;

^

../../buildtools/third_party/libc++/trunk/include/__config:271:9: error: unknown type name 'char32_t'
typedef
char32_t char32_t;

^

../../buildtools/third_party/libc++/trunk/include/__config:412:1: error: unknown type name 'namespace'
namespace std {
^
../../buildtools/third_party/libc++/trunk/include/__config:412:14: error: expected ';' after top level declarator
namespace std {

^

4 errors generated.

You shouldn't be including the C++ standard library using -I, especially if your doing it during C compilation. Chrome will continue to build if you replace -I path/to/libcxx/include with -cxx-isystem path/to/libcxx/include.

I'll continue to look into interactions between <__config> and extern "C", but it's simply incorrect to use libc++ during C compilation. There is a reason the clang driver doesn't add the libc++ include paths during C compilation and Chrome must do the same.

rsmith edited edge metadata.Oct 16 2015, 5:03 PM

if __config is first included within an extern "C" it will define the header guard macro but nothing else

That's not how this macro works: the __cplusplus macro is not affected by extern "C"; it will be defined for all C++ compilations, whether or not we're in an extern "C" context. Personally, I'm sympathetic to coping with libc++'s headers being present in C compilations; it may be hard to avoid this in some circumstances, with some build tools.

If we want the _LIBCPP_HAS_NO_SYSTEM_HEADER_PRAGMA macro (and friends) defined by <__config> even in C, we could put #ifdef __cplusplus around just the specific parts of this header that don't work in C (the namespaces, static_assert emulation, char16_t/char32_t, and the use of extern "C").

(If we don't want to support these headers being included from C, we should also remove the #ifdef __cplusplus checks from all the .h headers to avoid the suggestion that this is intended to work.)

if __config is first included within an extern "C" it will define the header guard macro but nothing else

That's not how this macro works: the __cplusplus macro is not affected by extern "C"; it will be defined for all C++ compilations, whether or not we're in an extern "C" context. Personally, I'm sympathetic to coping with libc++'s headers being present in C compilations; it may be hard to avoid this in some circumstances, with some build tools.

@aizatsky I'm sorry I think I got this wrong. I'm commit a patch similar to this after I audit how our C headers use <__config>. I think we can fix your issues with minimal pain to libc++.. However i'm not really happy having to worry about C compatibility.

aizatsky abandoned this revision.Nov 5 2015, 10:52 AM

Sorry for the delay. It should be fixed in r252274.