[PATCH][CMake] Fix PR14200, llvm-config output misses -fno-rtti

Fri Aug 14 02:02:03 PDT 2015

2015-08-14 2:13 GMT+02:00 Tom Stellard <tom at stellard.net>:
> On Thu, Aug 13, 2015 at 03:22:54PM -0700, Chris Bieneman wrote:
>> (+Tom Stellard)
>>
>> Looking at the patches, they’re probably the right way to go as a temporary fix. Certainly this is the change that should go into 3.7, so if we want this for 3.7 we should push it there. Any better fix will be bigger and riskier.
>>
>> Tom has been working on improving llvm-config recently, and has sent out a batch of patches (sorry for not getting to review them yet). In general I think Tom’s solution is closer to what we actually want to end up with.
>>
>> The code path in these patches makes me feel a little dirty because it uses get_property to read cxx flags of the target, but we set those flags based on variables in the llvm_update_compile_flags function. That whole flow just feels wrong to me, but I don’t think there is a simple solution. A better solution will probably involve refactoring (and maybe eliminating) llvm_update_compile_flags.
>>
>> TL;DR: These patches are fine to land, but we really should solve this in a better way in the not-so-distant future.
>>
>> Thoughts?
>
> I still don't know enough about CMake to comment on the implementation
> details, but I think a quick fix like this is really the only way we can
> get this fixed in the 3.7 branch, so I would be in favor of merging it.
>
> The correct solution requires some pretty invasive changes, like
> s/NDEBUG/LLVM_NDEBUG/ everywhere, and it will probably need to bake for
> a while in ToT to shake out all the issues.
>
> -Tom
>

Thanks for looking at the patch. I didn't push for landing it since I
noticed Tom's work which seems to be the correct long-term solution.
As I mentioned in my initial message I do not have commit rights so I
need some help with that part.

Best regards
David

>> -Chris
>>
>> > On Aug 13, 2015, at 2:43 PM, Chandler Carruth via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>> >
>> > Once we have the right solution, this should definitely go to the 3.7 branch.
>> >
>> > I'm not sure this is the best way to get the compile flags though. CC-ing Chris as he's probably the most immersed in this side of CMake at the moment.
>> >
>> > On Thu, Aug 13, 2015 at 9:55 AM Hans Wennborg <hans at chromium.org <mailto:hans at chromium.org>> wrote:
>> > On Sun, Aug 2, 2015 at 9:27 AM, David Wiberg <dwiberg at gmail.com <mailto:dwiberg at gmail.com>> wrote:
>> > > Hi,
>> > >
>> > > I took a stab at solving PR14200. My CMake knowledge is low but I think the
>> > > solution is reasonable as it reuses the existing logic for setting flags.
>> >
>> > Alexey: you're a cmake wizard. Can you take a look at this?
>> >
>> > Chandler: you're the cmake owner. I'd like to merge this to 3.7 when
>> > it lands if that's OK.
>> >
>> > > Things worth noting during review:
>> > > - The change means that both RTTI and exception handling flags will be
>> > > reported by llvm-config.
>> > > - I'm creating a variable with the same name as a target property. I don't
>> > > know if this breaks any convention.
>> >
>> > I tried this locally, and the output of llvm-config --cxxflags changed from:
>> >
>> >   -I/work/llvm/include -I/work/llvm/build.release/include  -fPIC
>> > -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter
>> > -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic
>> > -Wno-long-long -Wno-maybe-uninitialized -Wno-comment -std=c++11
>> > -ffunction-sections -fdata-sections -O3   -D_GNU_SOURCE
>> > -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
>> >
>> > to:
>> >
>> >   -I/work/llvm/include -I/work/llvm/build.release/include  -fPIC
>> > -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter
>> > -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic
>> > -Wno-long-long -Wno-maybe-uninitialized -Wno-comment -std=c++11
>> > -ffunction-sections -fdata-sections -O3  -fno-exceptions -fno-rtti
>> > -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
>> > -D__STDC_LIMIT_MACROS
>> >
>> > which seems correct.
>> >
>> > Thanks,
>> > Hans
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>