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

[reed kotler]

This situation occurs at the end of

bool CodeGenModule::TryEmitDefinitionAsAlias(GlobalDecl AliasDecl,
                                              GlobalDecl TargetDecl) {

apparently you can transfer attributes to an alias.

   // Finally, set up the alias with its proper name and attributes.
   SetCommonAttributes(cast<NamedDecl>(AliasDecl.getDecl()), Alias);

Nobody else is processing calls with such value pairs.

There are no mips target attributes in this example put apparently it is 
calling these target independent attribute processing methods anyway.

Perhaps I should do something which such calls but that would be a 
separate patch with it's own test case.

In this case, for sure I should be doing nothing because they are not 
even mips attributes.

On 03/18/2013 05:39 PM, David Blaikie wrote:
> 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?