On Mon, Mar 18, 2013 at 5:28 PM, reed kotler <[email protected]> wrote: > On 03/18/2013 05:18 PM, David Blaikie wrote: >> >> On Mon, Mar 18, 2013 at 3:18 PM, Reed Kotler <[email protected]> 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 >>> [email protected] >>> 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? _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
