[13] RFR: 8202414: Unsafe write after primitive array creation may result in array length change

[Rahul Raghavan]

Hi John,

Thank you, understood your point.
But please note here new code cannot be inserted at the other deleted 
code location as such, due to the dependency on 'offset'!

New code added / the main fix, can be added only after the line -
    AllocateNode* alloc = AllocateNode::Ideal_allocation(adr, phase, 
offset);
The deleted code / cleanup done, because new code added supersedes the 
checks.

(changeset committed -
http://hg.openjdk.java.net/jdk/jdk/rev/f8d2b5ce4491)

Thanks,
Rahul


On 01/05/19 11:50 PM, John Rose wrote:
> Here's a late comment:  Is there any reason to
> put the deletion and insertion in different
> places?  If not, it would be easier to follow
> the history, and to do merges, if they were
> placed at the same point in the code.
> That is, insert the new code where the old
> code is deleted.
> 
> On Apr 30, 2019, at 12:04 AM, Rahul Raghavan <rahul.v.raghavan at oracle.com> wrote:
>>
>> Thank you Vladimir Ivanov for suggestions.
>>
>> Please note following latest changes tried.
>> <webrev.04> - http://cr.openjdk.java.net/~rraghavan/8202414/webrev.04/
>>
>> Hope did not miss any points.
>> Confirmed no failures with the reported test cases.
>> Also hs-tier1 to tier4, hs-precheckin-comp testing in progress.
>>
>> Thanks,
>> Rahul
>>
>> On 27/04/19 11:48 AM, Vladimir Ivanov wrote:
>>> On 26/04/2019 19:30, Vladimir Ivanov wrote:
>>> After thinking more about it, I believe new offset alignment check supersedes is_unaligned_access(). And is_mismatched_access() is too conservative here: what is_mismatched_access() adds here (in addition to existing alignment & size checks) is whether type match between location and stored value, but what matters for IN are sizes and offsets only.
>>> Type mismatches (e.g., byte vs boolean, char vs short) may cause problems when consequent loads are replaced with values from initializing stores, but it should be already handled in MemNode::can_see_stored_value() and Load?Node::Ideal().
>>> So, it seems both checks (is_unaligned_access() & is_mismatched_access()) can be safely omitted.
>>> You are right, I missed that IN::captured_store_insertion_point() inspects already other stores which are already captured. Sorry for the confusion.
>>> I agree that IN::can_capture_store() is the right place to put the fix in and I like (iii). (Just add a comment, "// mismatched access" is enough)
>