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

[David Blaikie]

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