<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><br class="Apple-interchange-newline"><blockquote type="cite">----- Original Message -----<br><blockquote type="cite">From: "David Blaikie" <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>To: "Bill Schmidt" <<a href="mailto:wschmidt@linux.vnet.ibm.com">wschmidt@linux.vnet.ibm.com</a>><br>Cc: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>>, <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>, "Joe Abbey" <<a href="mailto:jabbey@arxan.com">jabbey@arxan.com</a>><br>Sent: Friday, November 16, 2012 1:19:30 PM<br>Subject: Re: [llvm-commits] Trivial warning<br><br>On Fri, Nov 16, 2012 at 11:13 AM, Bill Schmidt<br><<a href="mailto:wschmidt@linux.vnet.ibm.com">wschmidt@linux.vnet.ibm.com</a>> wrote:<br><blockquote type="cite">On Fri, 2012-11-16 at 12:32 -0600, Hal Finkel wrote:<br><blockquote type="cite">----- Original Message -----<br><blockquote type="cite">From: "Roman Divacky" <<a href="mailto:rdivacky@freebsd.org">rdivacky@freebsd.org</a>><br>To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>Cc: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>, "Joe Abbey" <<a href="mailto:jabbey@arxan.com">jabbey@arxan.com</a>><br>Sent: Friday, November 16, 2012 12:13:53 PM<br>Subject: Re: [llvm-commits] Trivial warning<br><br>On Fri, Nov 16, 2012 at 11:38:07AM -0600, Hal Finkel wrote:<br><blockquote type="cite">----- Original Message -----<br><blockquote type="cite">From: "Benjamin Kramer" <<a href="mailto:benny.kra@gmail.com">benny.kra@gmail.com</a>><br>To: "Joe Abbey" <<a href="mailto:jabbey@arxan.com">jabbey@arxan.com</a>><br>Cc: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>>, <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>Sent: Friday, November 16, 2012 10:00:47 AM<br>Subject: Re: [llvm-commits] Trivial warning<br><br><br>On 16.11.2012, at 13:11, Joe Abbey <<a href="mailto:jabbey@arxan.com">jabbey@arxan.com</a>> wrote:<br><br><blockquote type="cite">On Nov 16, 2012, at 12:38 AM, Joe Abbey <<a href="mailto:jabbey@arxan.com">jabbey@arxan.com</a>><br>wrote:<br><br><blockquote type="cite">Hal (et al),<br><br>One of my build bots reported a warning:<br>PPCRegisterInfo.cpp:501:51: warning: cast from type<br>???const<br>llvm::MachineFunction*??? to type<br>???llvm::MachineFunction*???<br>casts<br>away qualifiers [-Wcast-qual]<br>Index: lib/Target/PowerPC/PPCRegisterInfo.cpp<br>===================================================================<br>--- lib/Target/PowerPC/PPCRegisterInfo.cpp (revision<br>168110)<br>+++ lib/Target/PowerPC/PPCRegisterInfo.cpp (working<br>copy)<br>@@ -498,7 +498,8 @@<br> } else if (CRSpillFrameIdx) {<br> FrameIdx = CRSpillFrameIdx;<br> } else {<br>- MachineFrameInfo *MFI = ((MachineFunction<br>&)MF).getFrameInfo();<br>+ MachineFrameInfo *MFI =<br>+ (const_cast<MachineFunction<br>&>MF).getFrameInfo();<br> FrameIdx = MFI->CreateFixedObject((uint64_t)4,<br> (int64_t)-4,<br> true);<br> CRSpillFrameIdx = FrameIdx;<br> }<br><br>Ok to commit?<br></blockquote><br>Past Joe,<br><br>Make sure your current build is setup to do PowerPC<br>builds.<br><br>Also, swapping a warning for an error is a horrible<br>trade???<br>let's<br>try that patch again:<br><br>Index: lib/Target/PowerPC/PPCRegisterInfo.cpp<br>===================================================================<br>--- lib/Target/PowerPC/PPCRegisterInfo.cpp (revision<br>168110)<br>+++ lib/Target/PowerPC/PPCRegisterInfo.cpp (working<br>copy)<br>@@ -498,7 +498,8 @@<br> } else if (CRSpillFrameIdx) {<br> FrameIdx = CRSpillFrameIdx;<br> } else {<br>- MachineFrameInfo *MFI = ((MachineFunction<br>&)MF).getFrameInfo();<br>+ MachineFrameInfo *MFI =<br>+ (const_cast<MachineFunction<br>&>(MF)).getFrameInfo();<br> FrameIdx = MFI->CreateFixedObject((uint64_t)4,<br> (int64_t)-4,<br> true);<br> CRSpillFrameIdx = FrameIdx;<br> }<br></blockquote><br>While this would be the right way to silence the warning,<br>the<br>whole<br>thing looks like a horrible hack to me. Shouldn't the<br>FrameIdx<br>generated in a place where MF isn't const?<br></blockquote><br>Roman, you had looked at these const issues, right? Do you<br>have an<br>opinion on this?<br></blockquote><br>I actively avoided any const_cast<>s in my const rage :)<br><br>This code was contributed by wschmidt@ and iirc it needs to be<br>like this because PPC is special (addressing against fp).<br></blockquote><br>Bill, opinion?<br><br></blockquote><br>It looks to me like this has been changed a couple of times now:<br><br>Original code:<br> MachineFrameInfo *MFI = ((MachineFunction<br> &)MF).getFrameInfo();<br><br>Fixed in 165941 by Micah Villnow:<br><br>165941 mvillmow MachineFrameInfo *MFI =<br>(const_cast<MachineFunction &>(MF)).getFrameInfo();<br><br>Reverted by Chandler in 167222:<br><br>167222 chandlerc MachineFrameInfo *MFI = ((MachineFunction<br>&)MF).getFrameInfo();<br></blockquote><br>It looks like this particular change was reverted without prejudice.<br>The change was part of a much larger & more problematic change that<br>was reverted on those merits - this was just collateral damage.<br></blockquote><br>In that case, please (re-)apply the const_cast. Can someone please file a PR so that we don't forget to actually fix this problem.<br><br>Thanks again,<br>Hal<br></blockquote></div><div></div><blockquote type="cite"><div><br></div><div><span style="font-family: monospace; ">I think what this comes down to is a great big "group LGTM" to Joe for</span><br style="font-family: monospace; "><span style="font-family: monospace; ">his second attempt at the patch. :-D</span></div></blockquote><br><div>Very well. I'll commit the correct patch.</div><div><br></div><div>r168185</div><div></div><br><blockquote type="cite"><div><span style="font-family: monospace; ">While this would be the right way to silence the warning, the whole thing looks like a horrible hack to me. Shouldn't the FrameIdx generated in a place where MF isn't const?</span></div></blockquote><div><br></div><div>Benjamin, could you file a PR to un-hack the code?</div><div><br></div><div>Cheers,</div><div><br></div><div>Joe</div></body></html>