RFR: 8225779: Remove unused CollectedHeap::cell_header_size()

[Per Liden]

Thanks Erik!

/Per

On 6/18/19 5:09 PM, Erik Österlund wrote:
> Hi Per,
> 
> This new revision looks good to me.
> 
> /Erik
> 
> On 2019-06-17 23:36, Per Liden wrote:
>> Stefan pointed out that it looks like this piece of code can still 
>> pass in a negative offset (Type::OffsetBot = -2000000001) to 
>> MacroAssembler::needs_explicit_null_check(), which would cause the 
>> assert in that function to fail. I'm not hitting this in our testing, 
>> but it certainly looks suspicious.
>>
>> void PhaseCFG::implicit_null_check(Block* block, Node *proj, Node 
>> *val, int allowed_reasons) {
>>   [...]
>>       const Node* base = mach->get_base_and_disp(offset, adr_type);
>>       if (base == NULL || base == NodeSentinel) {
>>         // Narrow oop address doesn't have base, only index.
>>         // Give up if offset is beyond page size or if heap base is 
>> not protected.
>>         if (val->bottom_type()->isa_narrowoop() &&
>>             (MacroAssembler::needs_explicit_null_check(offset) ||
>>              !CompressedOops::use_implicit_null_checks()))
>>           continue;
>>   [...]
>> }
>>
>> Unless someone can explain that this can't happen, I think it would be 
>> safer to remove the assert and instead return true for all negative 
>> values, like we do today.
>>
>> Diff: http://cr.openjdk.java.net/~pliden/8225779/webrev.2-diff
>> Full: http://cr.openjdk.java.net/~pliden/8225779/webrev.2
>>
>> Opinions?
>>
>> cheers,
>> Per
>>
>> On 6/17/19 2:51 PM, Per Liden wrote:
>>> Here's an updated webrev, which just removes the cell_header_size() 
>>> and its uses, but otherwise keeps the code as is.
>>>
>>> http://cr.openjdk.java.net/~pliden/8225779/webrev.1
>>>
>>> cheers,
>>> Per
>>>
>>> On 6/14/19 1:43 PM, Per Liden wrote:
>>>> Hi,
>>>>
>>>> On 6/14/19 11:26 AM, Erik Österlund wrote:
>>>>> Hi Per,
>>>>>
>>>>> I don't think it is a good idea to completely revert the changes 
>>>>> made to MacroAssembler::needs_explicit_null_check(). This was not 
>>>>> just done for interfacing. The code we had before was actually wrong.
>>>>>
>>>>> We called that function with both addresses (from signal handlers) 
>>>>> and offsets (from the compiler), and tried to look at the passed in 
>>>>> value whether it quacks like an offset or an address based on its 
>>>>> value. If it looked like an address it would first be normalized to 
>>>>> look like an offset. But it was possible that a 
>>>>> much-larger-than-page offset is passed in that actually looks like 
>>>>> an address according to the address quacking code, which turns the 
>>>>> supposedly address looking value into an offset normalized form 
>>>>> that looks like it wouldn't need explicit null checks because in 
>>>>> that form the offset is within one page, while the original offset 
>>>>> passed in totally would need an explicit null check really, because 
>>>>> it totally wasn't within that one page offset we allowed.
>>>>>
>>>>> To make this more sound, the function was split into two - one used 
>>>>> for addresses passed in by signal handlers, and one used for 
>>>>> offsets used by the compiler. This completely removed the value 
>>>>> based check to determine if the argument looks like an address or 
>>>>> offset. I would definitely like to keep that structure going 
>>>>> forward, because otherwise that bug will be re-introduced.
>>>>
>>>> I see. I'll revise the patch.
>>>>
>>>> Thanks!
>>>> Per
>>>>
>>>>>
>>>>> Otherwise, I do encourage removing the now unnecessary cell header 
>>>>> size abstraction.
>>>>>
>>>>> /Erik
>>>>>
>>>>> On 2019-06-14 08:29, Per Liden wrote:
>>>>>> CollectedHeap::cell_header_size() is no longer used and can be 
>>>>>> removed. This patch is a hg backout of [1] where it was 
>>>>>> introduced, plus the two followup bug fixes [2] & [3].
>>>>>>
>>>>>> This is a clean backout, with the exception of light editing 
>>>>>> needed in two places, because:
>>>>>>
>>>>>> 1) The function Universe::narrow_oop_base() has since been renamed 
>>>>>> to CompressedOops::base().
>>>>>> 2) The include guard CPU_ARM_VM_MACROASSEMBLER_ARM_HPP has since 
>>>>>> been renamed to CPU_ARM_MACROASSEMBLER_ARM_HPP.
>>>>>>
>>>>>> Testing: Tier1-3 on all Oracle platforms
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8225779
>>>>>> Webrev: http://cr.openjdk.java.net/~pliden/8225779/webrev.0
>>>>>>
>>>>>> /Per
>>>>>>
>>>>>> [1] http://hg.openjdk.java.net/jdk/jdk/rev/4ad404da0088
>>>>>> [2] http://hg.openjdk.java.net/jdk/jdk/rev/f642ede2eb07
>>>>>> [3] http://hg.openjdk.java.net/jdk/jdk/rev/18bd95c0e463
>