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

[Per Liden]

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