[llvm-commits] Trivial warning

[Joe Abbey]

----- Original Message -----
From: "David Blaikie" <dblaikie at gmail.com<mailto:dblaikie at gmail.com>>
To: "Bill Schmidt" <wschmidt at linux.vnet.ibm.com<mailto:wschmidt at linux.vnet.ibm.com>>
Cc: "Hal Finkel" <hfinkel at anl.gov<mailto:hfinkel at anl.gov>>, llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>, "Joe Abbey" <jabbey at arxan.com<mailto:jabbey at arxan.com>>
Sent: Friday, November 16, 2012 1:19:30 PM
Subject: Re: [llvm-commits] Trivial warning

On Fri, Nov 16, 2012 at 11:13 AM, Bill Schmidt
<wschmidt at linux.vnet.ibm.com<mailto:wschmidt at linux.vnet.ibm.com>> wrote:
On Fri, 2012-11-16 at 12:32 -0600, Hal Finkel wrote:
----- Original Message -----
From: "Roman Divacky" <rdivacky at freebsd.org<mailto:rdivacky at freebsd.org>>
To: "Hal Finkel" <hfinkel at anl.gov<mailto:hfinkel at anl.gov>>
Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>, "Joe Abbey" <jabbey at arxan.com<mailto:jabbey at arxan.com>>
Sent: Friday, November 16, 2012 12:13:53 PM
Subject: Re: [llvm-commits] Trivial warning

On Fri, Nov 16, 2012 at 11:38:07AM -0600, Hal Finkel wrote:
----- Original Message -----
From: "Benjamin Kramer" <benny.kra at gmail.com<mailto:benny.kra at gmail.com>>
To: "Joe Abbey" <jabbey at arxan.com<mailto:jabbey at arxan.com>>
Cc: "Hal Finkel" <hfinkel at anl.gov<mailto:hfinkel at anl.gov>>, llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
Sent: Friday, November 16, 2012 10:00:47 AM
Subject: Re: [llvm-commits] Trivial warning


On 16.11.2012, at 13:11, Joe Abbey <jabbey at arxan.com<mailto:jabbey at arxan.com>> wrote:

On Nov 16, 2012, at 12:38 AM, Joe Abbey <jabbey at arxan.com<mailto:jabbey at arxan.com>>
wrote:

Hal (et al),

One of my build bots reported a warning:
PPCRegisterInfo.cpp:501:51: warning: cast from type
???const
llvm::MachineFunction*??? to type
???llvm::MachineFunction*???
casts
away qualifiers [-Wcast-qual]
Index: lib/Target/PowerPC/PPCRegisterInfo.cpp
===================================================================
--- lib/Target/PowerPC/PPCRegisterInfo.cpp   (revision
168110)
+++ lib/Target/PowerPC/PPCRegisterInfo.cpp   (working
copy)
@@ -498,7 +498,8 @@
    } else if (CRSpillFrameIdx) {
      FrameIdx = CRSpillFrameIdx;
    } else {
-      MachineFrameInfo *MFI = ((MachineFunction
&)MF).getFrameInfo();
+      MachineFrameInfo *MFI =
+        (const_cast<MachineFunction
&>MF).getFrameInfo();
      FrameIdx = MFI->CreateFixedObject((uint64_t)4,
      (int64_t)-4,
      true);
      CRSpillFrameIdx = FrameIdx;
    }

Ok to commit?

Past Joe,

Make sure your current build is setup to do PowerPC
builds.

Also, swapping a warning for an error is a horrible
trade???
let's
try that patch again:

Index: lib/Target/PowerPC/PPCRegisterInfo.cpp
===================================================================
--- lib/Target/PowerPC/PPCRegisterInfo.cpp    (revision
168110)
+++ lib/Target/PowerPC/PPCRegisterInfo.cpp    (working
copy)
@@ -498,7 +498,8 @@
    } else if (CRSpillFrameIdx) {
      FrameIdx = CRSpillFrameIdx;
    } else {
-      MachineFrameInfo *MFI = ((MachineFunction
&)MF).getFrameInfo();
+      MachineFrameInfo *MFI =
+        (const_cast<MachineFunction
&>(MF)).getFrameInfo();
      FrameIdx = MFI->CreateFixedObject((uint64_t)4,
      (int64_t)-4,
      true);
      CRSpillFrameIdx = FrameIdx;
    }

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?

Roman, you had looked at these const issues, right? Do you
have an
opinion on this?

I actively avoided any const_cast<>s in my const rage :)

This code was contributed by wschmidt@ and iirc it needs to be
like this because PPC is special (addressing against fp).

Bill, opinion?


It looks to me like this has been changed a couple of times now:

Original code:
     MachineFrameInfo *MFI = ((MachineFunction
     &)MF).getFrameInfo();

Fixed in 165941 by Micah Villnow:

165941   mvillmow       MachineFrameInfo *MFI =
(const_cast<MachineFunction &>(MF)).getFrameInfo();

Reverted by Chandler in 167222:

167222  chandlerc       MachineFrameInfo *MFI = ((MachineFunction
&)MF).getFrameInfo();

It looks like this particular change was reverted without prejudice.
The change was part of a much larger & more problematic change that
was reverted on those merits - this was just collateral damage.

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.

Thanks again,
Hal

I think what this comes down to is a great big "group LGTM" to Joe for
his second attempt at the patch. :-D

Very well.  I'll commit the correct patch.

r168185

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?

Benjamin, could you file a PR to un-hack the code?

Cheers,

Joe
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121116/fe65bcba/attachment.html>