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

[David Blaikie]

On Mon, Mar 18, 2013 at 5:28 PM, reed kotler <rkotler at mips.com> wrote:
> 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
>
> The fix may be to just eliminate the comment
>
>  // should not happen
>
> With my fix, everything works. Reverting my change will cause failures at Mips in our test-suite runs.

My point would be not that this patch should be reverted, but that the
original patch that added the buggy code be reverted until the bug/fix
is properly understood.

> That was just my opinion that I should not be called under these conditions.

& that seems to indicate that you don't have a clear understanding of
the code that's been written/committed - that doesn't seem like code
we would want committed, does it?