[llvm-commits] Trivial warning

[Benjamin Kramer]

On 16.11.2012, at 20:39, Joe Abbey <jabbey at arxan.com> wrote:

> 
> 
>> ----- Original Message -----
>>> From: "David Blaikie" <dblaikie at gmail.com>
>>> To: "Bill Schmidt" <wschmidt at linux.vnet.ibm.com>
>>> Cc: "Hal Finkel" <hfinkel at anl.gov>, llvm-commits at cs.uiuc.edu, "Joe Abbey" <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> wrote:
>>>> On Fri, 2012-11-16 at 12:32 -0600, Hal Finkel wrote:
>>>>> ----- Original Message -----
>>>>>> From: "Roman Divacky" <rdivacky at freebsd.org>
>>>>>> To: "Hal Finkel" <hfinkel at anl.gov>
>>>>>> Cc: llvm-commits at cs.uiuc.edu, "Joe Abbey" <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>
>>>>>>>> To: "Joe Abbey" <jabbey at arxan.com>
>>>>>>>> Cc: "Hal Finkel" <hfinkel at anl.gov>, 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> wrote:
>>>>>>>> 
>>>>>>>>> On Nov 16, 2012, at 12:38 AM, Joe Abbey <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?

http://llvm.org/bugs/show_bug.cgi?id=14364

- Ben