[cfe-dev] Small patches to allow fully independent clang/llvm/compiler-rt/libc++

[Jonathan Roelofs via cfe-dev]

On 10/14/15 8:53 AM, C Bergström wrote:
> On Wed, Oct 14, 2015 at 9:16 PM, Jonathan Roelofs
> <jonathan at codesourcery.com> wrote:
>>
>>
>> On 10/14/15 7:52 AM, C Bergström wrote:
>>>
>>> On Wed, Oct 14, 2015 at 8:47 PM, Jonathan Roelofs
>>> <jonathan at codesourcery.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/14/15 7:11 AM, Vasileios Kalintiris wrote:
>>>>>>
>>>>>>
>>>>>> Some people may disagree on the principle, even if it doesn't affect
>>>>>> current builds. But I'll let them voice their concerns.
>>>>>
>>>>>
>>>>>
>>>>> IMHO, the "correct" approach for a GNU-free/independent clang package
>>>>> requires
>>>>> a new ToolChain class for LLVM-based toolchains. Maybe, this class could
>>>>> inherit from the Linux ToolChain and use most of the existing
>>>>> infrastructure
>>>>> for paths, options & tools invocations. Otherwise, we could use these
>>>>> configuration-time options for most of the GNU-dependencies that we want
>>>>> to
>>>>> replace. However, I think that we would be duplicating
>>>>> effort/functionality
>>>>> at two different places (cmake & clang source code). Additionally, other
>>>>> than
>>>>> compiler-rt, libcxx, libcxxabi, etc., we'd need a non-GLIBC/GNU C
>>>>> library
>>>>> which might have a different set of CRT files with its own naming scheme
>>>>> for the
>>>>> dynamic linker/loader.
>>>>
>>>>
>>>>
>>>> Something along these ^ lines is the "correct" way to do it. You'd have
>>>> to
>>>> add your own vendor to Triple.h, and then specialize these defaults based
>>>> on
>>>> seeing that particular vendor.  Baking these decisions in at compile-time
>>>> is
>>>> an anti-pattern driver-wise.
>>>>
>>>> If you do it this way, then you'll be able to write testcases for your
>>>> changes, and those tests will get run by all the buildbots (not just one
>>>> configured to have one particular set of defaults).  This has the added
>>>> benefit of not requiring more builders to test more configurations.
>>>>
>>>> Please don't commit these patches as-is.
>>>
>>>
>>> Can you give precise feedback about what's wrong with the patches
>>> (as-is)? I'd like to "fix" them. Is it philosophical or something
>>> wrong?
>>
>>
>> To re-phrase, I think the "spirit" behind clang-01.diff, clang-02.diff and
>> clang-03.diff are contrary to the design goals of the driver. I think you
>> ought to re-work them so that they don't touch CMakeLists.txt at all, that
>> way all of these defaults are set at compiler runtime via the vendor part of
>> the triple (or some flag(s)), not at compiler compiletime.
>>
>> I'm not sure I'm the best person to review clang-04.diff, but at the very
>> least it needs testcases.
>
> The whole point of the patches is not to touch the default so "driver
> and flag people" can have it their way, but non-flag people can have a
> default. This doesn't hurt the driver and anyone using it would be
> opt-in.

I understand that's what you _want_ to do, but I still don't think 
that's the _right_ thing to do.

To name a specific example of undesirable effects of this series of 
patches, consider someone filing a bug report: in order to diagnose 
what's going on, now we'd need more than just the svn revision as 
printed in the version string... we'd need to know how the particular 
build was configured, which isn't something that the binary is able to 
tell us.

I'm pretty sure Eric (cc'd) would veto this patch for the same reason, 
but I'll let him state his own opinion on it.

>
> For 04 - can you give advice on how to make a testcase for it? I'm not
> 100% clear on how to test for a regression on this.

Since this requires that the driver be in the sysroot, you'll need to 
create a mock sysroot in `clang/test/Driver/Inputs`, and then have the 
testcase copy that to %T, set up a symlink from %T/bin/clang to the 
driver binary itself, then invoke the driver via that symlink, perform 
the checks on the -cc1 string, and finally clean everything up. 
Something like `clang/test/Driver/mips-fsf.cpp` would be a good start 
for comparison on the checks themselves.

On another note, I'm a little surprised there isn't existing testsuite 
coverage for these particular lines.


Jon

>

-- 
Jon Roelofs
jonathan at codesourcery.com
CodeSourcery / Mentor Embedded