This is an archive of the discontinued LLVM Phabricator instance.

Don't redefine a bunch of defines from llvm-config.h in config.h.
ClosedPublic

Authored by thakis on Apr 30 2018, 1:57 PM.

Download Raw Diff

Details

Reviewers

rnk
bogner
chapuni

Summary

r210144 made config.h include llvm-config.h and deduplicated defines. Then rL239987 later added back some of the duplication. http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150914/300329.html suggests this was done for the configure/make build, which no longer exists.

A prior attempt at this was apparently at https://reviews.llvm.org/D41371

Diff Detail

Event Timeline

thakis created this revision.Apr 30 2018, 1:57 PM

Herald added a subscriber: mgorny. · View Herald TranscriptApr 30 2018, 1:57 PM

missed one

bogner added inline comments.Apr 30 2018, 5:18 PM

include/llvm/Config/config.h.cmake
296–297	The one in llvm-config.h doesn't have this comment and does use cmakedefine. Please make sure we're still doing the right thing when this is empty (that is, build with -DLLVM_DEFAULT_TARGET_TRIPLE="")

thakis added inline comments.May 1 2018, 6:12 AM

include/llvm/Config/config.h.cmake
296–297	Since config.h includes llvm-config.h, when it's undefined in config.h the one in llvm-config.h won. So this is behavior-preserving, and since this doesn't work it looks like the comment is outdated.

bogner, rnk, chapuni: ping

include/llvm/Config/config.h.cmake
296–297	Did this make sense?

bogner added inline comments.May 2 2018, 3:21 PM

include/llvm/Config/config.h.cmake
296–297	No, I think you have this backwards. Today, if LLVM_DEFAULT_TARGET_TRIPLE is blank, llvm-config.h has a comment like so: /* #undef LLVM_DEFAULT_TARGET_TRIPLE */ Whereas config.h defines the value to an empty string: #define LLVM_DEFAULT_TARGET_TRIPLE "" So removing this from config.h will make it so that it's neither #defined nor #undef'd in either file, which is not behaviour preserving. In fact, if you build this way you'll probably get a compile error in lib/Support/Unix/Host.inc when it tries to call updateTripleOSVersion with no parameter. This is what I was asking you to check here. Moving the explicit define from here to llvm-config.h is also technically not behaviour preserving, since external projects that use these headers will see a #define of "" instead of no symbol at all, but I think that's at least a change that is arguably correct.

Address comment.

include/llvm/Config/config.h.cmake
296–297	Got it, thanks! I removed LLVM_DEFAULT_TARGET_TRIPLE from this change.

bogner accepted this revision.May 3 2018, 2:18 PM

This revision is now accepted and ready to land.May 3 2018, 2:18 PM

r331987, thanks!

Revision Contents

Path

Size

include/

llvm/

Config/

config.h.cmake

34 lines

Diff 144631

include/llvm/Config/config.h.cmake

	Show First 20 Lines • Show All 286 Lines • ▼ Show 20 Lines
	#cmakedefine HAVE____CHKSTK_MS ${HAVE____CHKSTK_MS}			#cmakedefine HAVE____CHKSTK_MS ${HAVE____CHKSTK_MS}

	/* Linker version detected at compile time. */			/* Linker version detected at compile time. */
	#cmakedefine HOST_LINK_VERSION "${HOST_LINK_VERSION}"			#cmakedefine HOST_LINK_VERSION "${HOST_LINK_VERSION}"

	/* Define if we link Polly to the tools */			/* Define if we link Polly to the tools */
	#cmakedefine LINK_POLLY_INTO_TOOLS			#cmakedefine LINK_POLLY_INTO_TOOLS

	/* Target triple LLVM will generate code for by default */
	/* Doesn't use `cmakedefine` because it is allowed to be empty. */
	#define LLVM_DEFAULT_TARGET_TRIPLE "${LLVM_DEFAULT_TARGET_TRIPLE}"
	bognerUnsubmitted Done Reply Inline Actions The one in llvm-config.h doesn't have this comment and does use cmakedefine. Please make sure we're still doing the right thing when this is empty (that is, build with -DLLVM_DEFAULT_TARGET_TRIPLE="") bogner: The one in llvm-config.h doesn't have this comment and does use cmakedefine. Please make sure…
	thakisAuthorUnsubmitted Done Reply Inline Actions Since config.h includes llvm-config.h, when it's undefined in config.h the one in llvm-config.h won. So this is behavior-preserving, and since this doesn't work it looks like the comment is outdated. thakis: Since config.h includes llvm-config.h, when it's undefined in config.h the one in llvm-config.h…
	thakisAuthorUnsubmitted Done Reply Inline Actions Did this make sense? thakis: Did this make sense?
	bognerUnsubmitted Done Reply Inline Actions No, I think you have this backwards. Today, if LLVM_DEFAULT_TARGET_TRIPLE is blank, llvm-config.h has a comment like so: /* #undef LLVM_DEFAULT_TARGET_TRIPLE / Whereas config.h defines the value to an empty string: #define LLVM_DEFAULT_TARGET_TRIPLE "" So removing this from config.h will make it so that it's neither #defined nor #undef'd in either file, which is not behaviour preserving. In fact, if you build this way you'll probably get a compile error in lib/Support/Unix/Host.inc when it tries to call updateTripleOSVersion with no parameter. This is what I was asking you to check here. Moving the explicit define from here to llvm-config.h is also technically not behaviour preserving, since external projects that use these headers will see a #define of "" instead of no symbol at all, but I think that's at least a change that is arguably correct. bogner:* No, I think you have this backwards. Today, if LLVM_DEFAULT_TARGET_TRIPLE is blank, llvm-config.
	thakisAuthorUnsubmitted Not Done Reply Inline Actions Got it, thanks! I removed LLVM_DEFAULT_TARGET_TRIPLE from this change. thakis: Got it, thanks! I removed LLVM_DEFAULT_TARGET_TRIPLE from this change.

	/* Define if threads enabled */
	#cmakedefine01 LLVM_ENABLE_THREADS

	/* Define if zlib compression is available */			/* Define if zlib compression is available */
	#cmakedefine01 LLVM_ENABLE_ZLIB			#cmakedefine01 LLVM_ENABLE_ZLIB

	/* Has gcc/MSVC atomic intrinsics */
	#cmakedefine01 LLVM_HAS_ATOMICS

	/* Host triple LLVM will be executed on */
	#cmakedefine LLVM_HOST_TRIPLE "${LLVM_HOST_TRIPLE}"

	/* Define if this is Unixish platform */
	#cmakedefine LLVM_ON_UNIX ${LLVM_ON_UNIX}

	/* Define if overriding target triple is enabled */			/* Define if overriding target triple is enabled */
	#cmakedefine LLVM_TARGET_TRIPLE_ENV "${LLVM_TARGET_TRIPLE_ENV}"			#cmakedefine LLVM_TARGET_TRIPLE_ENV "${LLVM_TARGET_TRIPLE_ENV}"

	/* Define if we have the Intel JIT API runtime support library */
	#cmakedefine01 LLVM_USE_INTEL_JITEVENTS

	/* Define if we have the oprofile JIT-support library */
	#cmakedefine01 LLVM_USE_OPROFILE

	/* LLVM version information */			/* LLVM version information */
	#cmakedefine LLVM_VERSION_INFO "${LLVM_VERSION_INFO}"			#cmakedefine LLVM_VERSION_INFO "${LLVM_VERSION_INFO}"

	/* Whether tools show host and target info when invoked with --version */			/* Whether tools show host and target info when invoked with --version */
	#cmakedefine01 LLVM_VERSION_PRINTER_SHOW_HOST_TARGET_INFO			#cmakedefine01 LLVM_VERSION_PRINTER_SHOW_HOST_TARGET_INFO

	/* Major version of the LLVM API */
	#define LLVM_VERSION_MAJOR ${LLVM_VERSION_MAJOR}

	/* Minor version of the LLVM API */
	#define LLVM_VERSION_MINOR ${LLVM_VERSION_MINOR}

	/* Patch version of the LLVM API */
	#define LLVM_VERSION_PATCH ${LLVM_VERSION_PATCH}

	/* LLVM version string */
	#define LLVM_VERSION_STRING "${PACKAGE_VERSION}"

	/* Define if libxml2 is supported on this platform. */			/* Define if libxml2 is supported on this platform. */
	#cmakedefine LLVM_LIBXML2_ENABLED ${LLVM_LIBXML2_ENABLED}			#cmakedefine LLVM_LIBXML2_ENABLED ${LLVM_LIBXML2_ENABLED}

	/* Define to the extension used for shared libraries, say, ".so". */			/* Define to the extension used for shared libraries, say, ".so". */
	#cmakedefine LTDL_SHLIB_EXT "${LTDL_SHLIB_EXT}"			#cmakedefine LTDL_SHLIB_EXT "${LTDL_SHLIB_EXT}"

	/* Define to the address where bug reports for this package should be sent. */			/* Define to the address where bug reports for this package should be sent. */
	#cmakedefine PACKAGE_BUGREPORT "${PACKAGE_BUGREPORT}"			#cmakedefine PACKAGE_BUGREPORT "${PACKAGE_BUGREPORT}"
	Show All 29 Lines