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

Thu Dec 13 15:41:11 PST 2018

On 12/13/2018 02:06 PM, Finkel, Hal J. wrote:
> 
> 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 understand your point here, and at least we are moving in the right
direction with this patch which removes almost all of the cxxflags from
llvm-config.

To be honest, I don't really have a strong opinion about keeping these flags,
I just felt like it would better to keep the flags and that anyone
negatively impacted by them could just not use llvm-config --cxxflags,
and instead use llvm-config --cppflags and then hard-code -std=c++11, which
is the only other thing llvm-config --cxxflags reports.

If everyone is complaining about too many flags in llvm-config, then I
wonder what useful information they are even getting out of it?  Is
it just the cppflags?  What do you think is the minimal set of flags
it could report and still be useful?

-Tom

> 
>>
>> 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
> 