[llvm] r184793 - Remove all non-linker oriented compile options from the linker

[Bob Wilson]

On Jun 26, 2013, at 11:55 PM, Eric Christopher <echristo at gmail.com> wrote:

> Yeah. No matter what that isn't the correct fix. Can you be more clear on what the symptoms you saw are and what's going on? I've been communicating with Michael since committing and was under the impression that all of them were fixed.
> 
> -eric
> 

Michael saw the failure but wasn't sure if it was caused by your change, so he passed it on to me.  I can now confirm that adding CXXFLAGS fixed the buildbot.

The issue here is that there are a variety of command-line options for the compiler that apply to both compile and link steps in the build.  It is useful to have a single variable that can be used for those settings.  Specifically, one of our builds involves running make with CFLAGS and CXXFLAGS set on the command line to add a "-arch" option.  Your change broke that.  We could probably fix it by changing our build script to also add LDFLAGS, but I don't understand why you want to make this change.

The tool being invoked by the "Link" command in the makefile is "$(CXX)".  It makes perfect sense for that command to include "$(CXXFLAGS)" on the command line.  The name of that variable implies that it contains options for $CXX.  It's not called $COMPILE_FLAGS.  If we're going to do something here, I think it should be to rename or replace the LDFLAGS variable, since in other contexts, that variable is typically used to specify options to be passed to ld.  Why are we passing "$(LDFLAGS)" to the compiler?

At some level, this boils down to an argument about naming conventions, and I don't think there is a clear right or wrong answer.  More importantly, it is fairly common for people to set CFLAGS and CXXFLAGS when building.  Your change broke that for Apple, and it could well affect other LLVM users.  What is the motivation for introducing this incompatibility with previous LLVM releases?  If there's a good reason for it, I don't mind fixing our builds to accommodate it, but I don't think it's worth the trouble and risk of breaking other people's builds without a clear benefit to changing things.

> On Jun 26, 2013 11:13 PM, "Bob Wilson" <bob.wilson at apple.com> wrote:
> This broke one of Apple’s buildbots.  I have tried to get it going again by adding CXXFLAGS back to the link command in r185060.  I still need to confirm that it actually fixes the buildbot.  Assuming that it does, will that work for you, Eric?  It wasn’t clear from your commit message why you changed this, so I don’t know what your constraints are.
> 
> On Jun 24, 2013, at 4:20 PM, Eric Christopher <echristo at gmail.com> wrote:
> 
>> Author: echristo
>> Date: Mon Jun 24 18:20:04 2013
>> New Revision: 184793
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=184793&view=rev
>> Log:
>> Remove all non-linker oriented compile options from the linker
>> command line. Change the darwin universal binary options to
>> be TargetCommonOpts so that they'll be passed to the linker since
>> -arch at least is still needed.
>> 
>> Someone on darwin with a buildit based build should probably verify
>> that this doesn't break anything there.
>> 
>> Modified:
>>    llvm/trunk/Makefile.rules
>> 
>> Modified: llvm/trunk/Makefile.rules
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/Makefile.rules?rev=184793&r1=184792&r2=184793&view=diff
>> ==============================================================================
>> --- llvm/trunk/Makefile.rules (original)
>> +++ llvm/trunk/Makefile.rules Mon Jun 24 18:20:04 2013
>> @@ -691,9 +691,9 @@ ifdef UNIVERSAL
>>     UNIVERSAL_ARCH := i386 ppc
>>   endif
>>   UNIVERSAL_ARCH_OPTIONS := $(UNIVERSAL_ARCH:%=-arch %)
>> -  CompileCommonOpts += $(UNIVERSAL_ARCH_OPTIONS)
>> +  TargetCommonOpts += $(UNIVERSAL_ARCH_OPTIONS)
>>   ifdef UNIVERSAL_SDK_PATH
>> -    CompileCommonOpts += -isysroot $(UNIVERSAL_SDK_PATH)
>> +    TargetCommonOpts += -isysroot $(UNIVERSAL_SDK_PATH)
>>   endif
>> 
>>   # Building universal cannot compute dependencies automatically.
>> @@ -755,8 +755,7 @@ Preprocess.CXX= $(Compile.Wrapper) \
>> 	          $(CXX) $(CPP.Flags) $(TargetCommonOpts) $(CPPFLAGS) \
>>                 $(CompileCommonOpts) $(CXX.Flags) -E
>> Link          = $(Compile.Wrapper) \
>> -	          $(CXX) $(CPP.Flags) $(CXX.Flags) $(CXXFLAGS) $(LD.Flags) \
>> -                $(LDFLAGS) $(TargetCommonOpts)  $(CompileCommonOpts) $(Strip)
>> +	          $(CXX) $(LD.Flags) $(LDFLAGS) $(TargetCommonOpts) $(Strip)
>> 
>> BCCompile.C   = $(LLVMCC) $(CPP.Flags) $(C.Flags) $(CFLAGS) $(CPPFLAGS) \
>>                 $(TargetCommonOpts) $(CompileCommonOpts)
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130627/5c3a14e7/attachment.html>