[PATCH, RFC] Fix PR16454

[Bill Schmidt]

On Fri, 2013-06-28 at 12:18 -0700, Chad Rosier wrote:
> 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, thanks!  I'll have a look at that.

Bill

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