r177329 - This code works around what appears to be a bug in another part of clang.

[reed kotler]

On 03/18/2013 05:18 PM, David Blaikie wrote:
> On Mon, Mar 18, 2013 at 3:18 PM, Reed Kotler <rkotler at mips.com> wrote:
>> Author: rkotler
>> Date: Mon Mar 18 17:18:00 2013
>> New Revision: 177329
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=177329&view=rev
>> Log:
>> This code works around what appears to be a bug in another part of clang.
>> I have filed http://llvm.org/bugs/show_bug.cgi?id=15538 against clang.
>> This code is safer anyway because "cast" assumes you really know that
>> it's okay to make the cast. In this case isa should not be false and
>> dyn_cast should not return null as far as I understand. But everything
>> else is valid so I did not want to revert my previous patch for attributes
>> mips16/nomips16 or use an llvm_unreachable here which would make a number
>> of our tests fail for mips.
>>
>>
>> Modified:
>>      cfe/trunk/lib/CodeGen/TargetInfo.cpp
>>
>> Modified: cfe/trunk/lib/CodeGen/TargetInfo.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=177329&r1=177328&r2=177329&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/CodeGen/TargetInfo.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/TargetInfo.cpp Mon Mar 18 17:18:00 2013
>> @@ -4323,7 +4323,8 @@ public:
>>                              CodeGen::CodeGenModule &CGM) const {
>>       const FunctionDecl *FD = dyn_cast<FunctionDecl>(D);
>>       if (!FD) return;
>> -    llvm::Function *Fn = cast<llvm::Function>(GV);
>> +    llvm::Function *Fn = dyn_cast<llvm::Function>(GV);
>> +    if (!Fn) return; // should not happen
> Do you have a test case to commit with this?
>
> Also - I tend to agree with Rafael: if you don't know why you need to
> make a change, then it's probably not OK to make this fix. Sounds like
> you don't understand what's going on here & are papering over it in a
> way that seems to work but without any basis to believe that your
> change is right/correct other than "it works". We don't really develop
> code this way.
>
> If your reviewers told you to write it some way that doesn't work,
> discuss it with them.
>
> If other targets have the same bug, that's not necessarily sufficient
> justification to add another instance of the same bug once we know it
> is a bug.
>
> We aren't punishing you for being honest, we're asking you to not
> commit buggy/insufficiently understood/justified code.
>
> Checking in an uncertain fix while you investigate the problem isn't
> the usual practice on the project - we tend to revert a patch until we
> can it's correct. (granted, if a bug has been in the codebase long
> enough we don't trawl back through the commits & rip out the
> patch/functionality that added it - so, yes, the longer a patch is in
> the more likely that when we find a bug we'll just let the tree
> continue to be broken until we commit a fix for the bug - in this case
> it sounds like your contribution is still fresh enough to be a fix
> (with a known correct fix) or revert kind of situation)
>
>>       if (FD->hasAttr<Mips16Attr>()) {
>>         Fn->addFnAttr("mips16");
>>       }
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

With my fix, everything works. Reverting my change will cause failures 
at Mips in our test-suite runs. Not a lot of failures but some 
compilation failures with c++ exception handling it seems.

I am investigating this more thoroughly now to understand if my original 
comment was incorrect, maybe it can be called with an functional 
declaration and global value which is not  function or maybe there is a 
bug in clang.