[PATCH, RFC] Fix PR16454

[Chad Rosier]

Hi Bill,
I'm not sure if this will be helpful, but what happens if you check to make sure you're only performing a single JobAction?  If the -save-temps option is being specified, then I assume there will be multiple jobs.

I did something kinda like this in:
http://llvm.org/viewvc/llvm-project?view=revision&revision=143813

 Chad

On Jun 27, 2013, at 8:47 AM, Bill Schmidt <wschmidt at linux.vnet.ibm.com> wrote:

> On Wed, 2013-06-26 at 16:54 -0500, Bill Schmidt wrote:
>> 
>> On Wed, 2013-06-26 at 16:14 -0500, Hal Finkel wrote:
>>> ----- Original Message -----
>>>> http://llvm.org/bugs/show_bug.cgi?id=16454 demonstrates that
>>>> altivec.h
>>>> is being included along paths where it doesn't make much sense to do
>>>> so.
>>>> This file is auto-included when compiling with -faltivec.  What I'm
>>>> proposing is to avoid copying the -faltivec flag when the action
>>>> being
>>>> taken is to invoke the assembler or the preprocessor.
>>>> 
>>>> I've verified that this fixes the test case in question without
>>>> introducing any unit-test or test-suite regressions.  I also verified
>>>> that preprocessing a file requiring altivec extensions (like "vector
>>>> float x;") with the revised Clang and then compiling the resulting
>>>> preprocessed file still works correctly.
>>>> 
>>>> However, I really don't know anything about how all the driver
>>>> machinery
>>>> works, so I'd appreciate some extra eyes on this before I commit.
>>>> I'm
>>>> CC'ing a few folks that were suggested.
>>>> 
>>>> For what it's worth, the test case in question is fixed by the
>>>> isa<PreprocessJobAction> test.  The other test is there just because
>>>> it
>>>> makes sense to me.
>>>> 
>>>> I will add the original test from the PR as a unit test if this
>>>> approach
>>>> passes muster.
>>> 
>>> I think that this makes sense; so the job action is not available from within Frontend/CompilerInvocation.cpp where the file is actually included?
>> 
>> Right, at least so far as I can tell.  That code doesn't appear to be
>> aware whence it was called.
>> 
>>> 
>>> To confirm, if you compile something with clang that crashes, thus generating a preprocessed input file, the contents of altivec.h will no longer be copied into that file, correct?
>> 
>> Well, this is interesting.  I get different behavior with and without
>> --save-temps:
>> 
>> $ cat crashit.c
>> vector float zowie wheresmycomma;
>> 
>> $ $LLVM_INSTALL/bin/clang -E -maltivec crashit.c
>> # 1 "crashit.c"
>> # 1 "<built-in>" 1
>> # 1 "<built-in>" 3
>> # 155 "<built-in>" 3
>> # 1 "<command line>" 1
>> # 1 "<built-in>" 2
>> # 1 "crashit.c" 2
>> vector float zowie wheresmycomma;
>> 
>> $ $LLVM_INSTALL/bin/clang -c -maltivec crashit.c
>> crashit.c:1:19: error: expected ';' after top level declarator
>> vector float zowie wheresmycomma;
>>                  ^
>>                  ;
>> 1 error generated.
>> 
>> $ $LLVM_INSTALL/bin/clang -c -maltivec --save-temps crashit.c
>> In file included from <built-in>:158:
>> <command line>:1:10: fatal error: 'altivec.h' file not found
>> #include "altivec.h"
>>         ^
>> 1 error generated.
>> 
>> 
>> I guess this is why I need a driver expert.  Can anyone explain what
>> code path we're going down with --save-temps that causes the different
>> behavior?  (BTW, --save-temps produces a crashit.i identical to the
>> output from the -E command.)
> 
> So, the patch is not acceptable as is; it introduces an error with
> --save-temps even for correct code:
> 
> $ cat ok.c
> vector float zowie, wheresmycomma;
> $ $LLVM_INSTALL/bin/clang -c -maltivec ok.c
> $ $LLVM_INSTALL/bin/clang -c -maltivec --save-temps ok.c
> In file included from <built-in>:158:
> <command line>:1:10: fatal error: 'altivec.h' file not found
> #include "altivec.h"
>         ^
> 1 error generated.
> 
> 
> I am not certain how to proceed.  Suggestions from driver experts
> welcome.
> 
> Thanks,
> Bill
> 
>> 
>> Thanks, Hal!
>> 
>> Bill
>> 
>>> 
>>> -Hal
>>> 
>>>> 
>>>> Thanks,
>>>> Bill
>>>> 
>>>> 
>>>> Index: lib/Driver/Tools.cpp
>>>> ===================================================================
>>>> --- lib/Driver/Tools.cpp	(revision 184947)
>>>> +++ lib/Driver/Tools.cpp	(working copy)
>>>> @@ -2844,7 +2844,10 @@ void Clang::ConstructJob(Compilation &C, const
>>>> Job
>>>>   Args.AddLastArg(CmdArgs, options::OPT_flimit_debug_info);
>>>>   Args.AddLastArg(CmdArgs, options::OPT_fno_limit_debug_info);
>>>>   Args.AddLastArg(CmdArgs, options::OPT_fno_operator_names);
>>>> -  Args.AddLastArg(CmdArgs, options::OPT_faltivec);
>>>> +  // -faltivec causes inclusion of altivec.h, which makes no sense
>>>> for
>>>> +  // an assembly or preprocess action.
>>>> +  if (!isa<AssembleJobAction>(JA) && !isa<PreprocessJobAction>(JA))
>>>> +    Args.AddLastArg(CmdArgs, options::OPT_faltivec);
>>>>   Args.AddLastArg(CmdArgs,
>>>>   options::OPT_fdiagnostics_show_template_tree);
>>>>   Args.AddLastArg(CmdArgs, options::OPT_fno_elide_type);
>>>> 
>>>> 
>>>> 
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>> 
>>> 
>