<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">I have proposed a patch to clang in
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<a href="http://llvm.org/bugs/show_bug.cgi?id=15538">http://llvm.org/bugs/show_bug.cgi?id=15538</a><br>
<br>
Do you want me to submit this patch?<br>
<br>
For now I will make the test case for my patch and remove the
offending comment.<br>
<br>
<br>
On 03/18/2013 05:39 PM, David Blaikie wrote:<br>
</div>
<blockquote
cite="mid:CAENS6EsVhc1TopGWAenk_VSO-FgeMA21xqHn1NpJSH7nT=3qpg@mail.gmail.com"
type="cite">
<pre wrap="">On Mon, Mar 18, 2013 at 5:28 PM, reed kotler <a class="moz-txt-link-rfc2396E" href="mailto:rkotler@mips.com"><rkotler@mips.com></a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On 03/18/2013 05:18 PM, David Blaikie wrote:
</pre>
<blockquote type="cite">
<pre wrap="">
On Mon, Mar 18, 2013 at 3:18 PM, Reed Kotler <a class="moz-txt-link-rfc2396E" href="mailto:rkotler@mips.com"><rkotler@mips.com></a> wrote:
</pre>
<blockquote type="cite">
<pre wrap="">
Author: rkotler
Date: Mon Mar 18 17:18:00 2013
New Revision: 177329
URL: <a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project?rev=177329&view=rev">http://llvm.org/viewvc/llvm-project?rev=177329&view=rev</a>
Log:
This code works around what appears to be a bug in another part of clang.
I have filed <a class="moz-txt-link-freetext" href="http://llvm.org/bugs/show_bug.cgi?id=15538">http://llvm.org/bugs/show_bug.cgi?id=15538</a> 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:
<a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=177329&r1=177328&r2=177329&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/TargetInfo.cpp?rev=177329&r1=177328&r2=177329&view=diff</a>
==============================================================================
--- 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
</pre>
</blockquote>
<pre wrap="">
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)
</pre>
<blockquote type="cite">
<pre wrap=""> if (FD->hasAttr<Mips16Attr>()) {
Fn->addFnAttr("mips16");
}
_______________________________________________
cfe-commits mailing list
<a class="moz-txt-link-abbreviated" href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a>
<a class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a>
</pre>
</blockquote>
</blockquote>
<pre wrap="">
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.
</pre>
</blockquote>
<pre wrap="">
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.
</pre>
<blockquote type="cite">
<pre wrap="">That was just my opinion that I should not be called under these conditions.
</pre>
</blockquote>
<pre wrap="">
& 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?
</pre>
</blockquote>
<br>
</body>
</html>