[llvm] r349068 - Don't add unnecessary compiler flags to llvm-config output

Thu Dec 13 14:06:34 PST 2018

On 12/13/18 2:48 PM, Tom Stellard wrote:
> On 12/13/2018 10:45 AM, Finkel, Hal J. wrote:
>> Hi, Tom,
>>
>> Are -fno-exceptions -fno-rtti also necessary? My students this quarter
>> complained about this quite a bit ;)
>>
> The philosophy here was to include flags that affect the ABI of the
> dynamic library and that were added explicitly by LLVM's build system.

There's a significant different between flags necessary to *use* the ABI
of the library and the flags necessary to *reproduce* the ABI of the
library. These flags are for the latter, not the former (AFAIK).

>
> It's true that some programs that need rtti will break if they use
> llvm-config --cxxflags throughout their entire program, but at
> least the error will be coming from their own code.  If
> we omit the -fno-rtti flag and someone is trying to use rtti
> of LLVM objects then the error will appear to be in the LLVM code and
> then it looks like an LLVM bug.

I understand the philosophy, but it doesn't work well in practice. Also,
IMHO, the -fno-exceptions are even more problematic than the -fno-rtti.
What turned out to be true was that nearly every project of non-trivial
size ended up employing some work-around for this issue, either
post-processing the output of llvm-config with sed (or similar),
hard-coding the output from llvm-config into the project build system
(which is obviously fragile), etc. This all makes LLVM unnecessarily
painful to use. In short, if we want to subset C++ for our own purposes,
that's fine, but we shouldn't be exporting that decision to our users.
While that might cause the LLVM build to appear to be buggy in certain
cases, in my experience, the overall usability impediment is much more
serious. Build systems tend to combine all of the relevant build flags
together, so this obviously ends up affecting lots of code that has
nothing to do with LLVM.

>
> I think what we have now is a good combination of simplicity and
> user-friendliness.

Unfortunately, I disagree. So long as we include -fno-exceptions
-fno-rtti in the output, we'll remain on the wrong side of generally
useful. I'm definitely happy to see these flags cleaned up, but unless
we address this problem, I don't think we'll have significantly
increased the utility of llvm-config.

Thanks again,

Hal

>   The reality is that users don't have to use
> llvm-config and can always craft their own cxxflags.  This should
> be much easier now that llvm-config --cxxflags only reports 1-3 
> compiler flags instead of 20+.
>
> -Tom
>
>
>>  -Hal
>>
>> On 12/13/18 12:21 PM, Tom Stellard via llvm-commits wrote:
>>> Author: tstellar
>>> Date: Thu Dec 13 10:21:23 2018
>>> New Revision: 349068
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=349068&view=rev
>>> Log:
>>> Don't add unnecessary compiler flags to llvm-config output
>>>
>>> Summary:
>>> llvm-config --cxxflags --cflags, should only output the minimal flags
>>> required to link against the llvm libraries.  They currently contain
>>> all flags used to compile llvm including flags like -g, -pedantic,
>>> -Wall, etc, which users may not always want.
>>>
>>> This changes the llvm-config output to only include flags that have been
>>> explictly added to the COMPILE_FLAGS property of the llvm-config target
>>> by the llvm build system.
>>>
>>> llvm.org/PR8220
>>>
>>> Output from llvm-config when running cmake with:
>>> cmake -G Ninja .. -DCMAKE_CXX_FLAGS=-funroll-loops
>>>
>>> Before:
>>>
>>> --cppflags: -I$HEADERS_DIR/llvm/include -I$HEADERS_DIR/llvm/build/include
>>>             -D_GNU_SOURCE -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
>>> --cflags:   -I$HEADERS_DIR/llvm/include -I$HEADERS_DIR/llvm/build/include
>>>             -fPIC -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings \
>>>             -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough \
>>>             -Wno-comment -fdiagnostics-color -g -D_GNU_SOURCE -D_DEBUG -D__STDC_CONSTANT_MACROS \
>>>             -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
>>> --cxxflags: -I$HEADERS_DIR/llvm/include -I$HEADERS_DIR/llvm/build/include\
>>>             -funroll-loops -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall \
>>>             -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers \
>>>             -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized \
>>>             -Wno-class-memaccess -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment \
>>>             -fdiagnostics-color -g  -fno-exceptions -fno-rtti -D_GNU_SOURCE -D_DEBUG \
>>>             -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS"
>>>
>>> After:
>>>
>>> --cppflags: -I$HEADERS_DIR/llvm/include -I$HEADERS_DIR/llvm/build/include \
>>>             -D_GNU_SOURCE -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
>>> --cflags:   -I$HEADERS_DIR/llvm/include -I$HEADERS_DIR/llvm/build/include \
>>>             -D_GNU_SOURCE -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
>>> --cxxflags: -I$HEADERS_DIR/llvm/include -I$HEADERS_DIR/llvm/build/include \
>>>              -std=c++11   -fno-exceptions -fno-rtti \
>>>              -D_GNU_SOURCE -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
>>>
>>> Reviewers: sylvestre.ledru, infinity0, mgorny
>>>
>>> Reviewed By: sylvestre.ledru, mgorny
>>>
>>> Subscribers: mgorny, dmgreen, llvm-commits
>>>
>>> Differential Revision: https://reviews.llvm.org/D55391
>>>
>>> Modified:
>>>     llvm/trunk/tools/llvm-config/CMakeLists.txt
>>>
>>> Modified: llvm/trunk/tools/llvm-config/CMakeLists.txt
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-config/CMakeLists.txt?rev=349068&r1=349067&r2=349068&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/tools/llvm-config/CMakeLists.txt (original)
>>> +++ llvm/trunk/tools/llvm-config/CMakeLists.txt Thu Dec 13 10:21:23 2018
>>> @@ -29,12 +29,20 @@ string(REPLACE ";" " " SYSTEM_LIBS "${SY
>>>  # Fetch target specific compile options, e.g. RTTI option
>>>  get_property(COMPILE_FLAGS TARGET llvm-config PROPERTY COMPILE_FLAGS)
>>>  
>>> +# The language standard potentially affects the ABI/API of LLVM, so we want
>>> +# to make sure it is reported by llvm-config.
>>> +# NOTE: We don't want to start extracting any random C/CXX flags that the
>>> +# user may add that could affect the ABI.  We only want to extract flags
>>> +# that have been added by the LLVM build system.
>>> +string(REGEX MATCH "-std=[^ ]\+" LLVM_CXX_STD_FLAG ${CMAKE_CXX_FLAGS})
>>> +string(REGEX MATCH "-std=[^ ]\+" LLVM_C_STD_FLAG ${CMAKE_C_FLAGS})
>>> +
>>>  # Use configure_file to create BuildVariables.inc.
>>>  set(LLVM_SRC_ROOT ${LLVM_MAIN_SRC_DIR})
>>>  set(LLVM_OBJ_ROOT ${LLVM_BINARY_DIR})
>>> -set(LLVM_CPPFLAGS "${CMAKE_CPP_FLAGS} ${CMAKE_CPP_FLAGS_${uppercase_CMAKE_BUILD_TYPE}} ${LLVM_DEFINITIONS}")
>>> -set(LLVM_CFLAGS "${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${uppercase_CMAKE_BUILD_TYPE}} ${LLVM_DEFINITIONS}")
>>> -set(LLVM_CXXFLAGS "${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_${uppercase_CMAKE_BUILD_TYPE}} ${COMPILE_FLAGS} ${LLVM_DEFINITIONS}")
>>> +set(LLVM_CPPFLAGS "${LLVM_DEFINITIONS}")
>>> +set(LLVM_CFLAGS "${LLVM_C_STD_FLAG} ${LLVM_DEFINITIONS}")
>>> +set(LLVM_CXXFLAGS "${LLVM_CXX_STD_FLAG} ${COMPILE_FLAGS} ${LLVM_DEFINITIONS}")
>>>  set(LLVM_BUILD_SYSTEM cmake)
>>>  set(LLVM_HAS_RTTI ${LLVM_CONFIG_HAS_RTTI})
>>>  set(LLVM_DYLIB_VERSION "${LLVM_VERSION_MAJOR}${LLVM_VERSION_SUFFIX}")
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory