RFR: aarch32: ARM 32 build broken after 8165929

[Chris Phillips]

Hi,

On 15/03/18 05:55 PM, coleen.phillimore at oracle.com wrote:
> 
> Thank you for fixing this!  I don't know arm32 assembly but I'm happy
> that you've fixed the inconsistency.
> 
> Coleen
> 
> On 3/15/18 5:40 PM, Edward Nevill wrote:
>> Hi,
>>
>> Please review the following webrev
>>
>> Bugid: https://bugs.openjdk.java.net/browse/JDK-8199243
>> Webrev: http://cr.openjdk.java.net/~enevill/8199243/webrev.00
>>
>> The ARM 32 build is broken with multiple errors of the form
>>
>> /work/ed/arm32/jdk/src/hotspot/os_cpu/linux_arm/copy_linux_arm.inline.hpp:33:54:
>> error: invalid conversion from 'const HeapWord*' to 'HeapWord*'
>> [-fpermissive]
>>     _Copy_conjoint_words(to, from, count * HeapWordSize);
>>
>> The problem was introduced by change 8165929
>>
>> # HG changeset patch
>> # User coleenp
>> # Date 1518182622 18000
>> #      Fri Feb 09 08:23:42 2018 -0500
>> # Node ID 950c35ea6237afd834d02345a2878e5dc30750e0
>> # Parent  f323537c9b75444578c75d348fa2e5be81532d3e
>> 8165929: Constify arguments of Copy methods
>> Reviewed-by: hseigel, kbarrett
>>
>> This change added 'const' to the 'from' arguments in various Copy
>> functions, for example
>>
>> -  void _Copy_conjoint_words(HeapWord* from, HeapWord* to, size_t count);
>> -  void _Copy_disjoint_words(HeapWord* from, HeapWord* to, size_t count);
>> +  void _Copy_conjoint_words(const HeapWord* from, HeapWord* to,
>> size_t count);
>> +  void _Copy_disjoint_words(const HeapWord* from, HeapWord* to,
>> size_t count);
>>
>> The problem in the ARM 32 port occurs in code like the following
>>
>> static void pd_conjoint_words(const HeapWord* from, HeapWord* to,
>> size_t count) {
>> #ifdef AARCH64
>>    _Copy_conjoint_words(from, to, count * HeapWordSize);
>> #else
>>     // NOTE: _Copy_* functions on 32-bit ARM expect "to" and "from"
>> arguments in reversed order
>>    _Copy_conjoint_words(to, from, count * HeapWordSize);
>> #endif
>> }
>>
>> The assembler implementation of the Copy functions in ARM 32 actually
>> copies in the wrong order. Ie it copies from 'to' and to 'from'.
>>
>> Looking at the assembler implementation it says the following
>>
>>         # Support for void Copy::conjoint_words(void* from,
>>          #                                       void* to,
>>          #                                       size_t count)
>> _Copy_conjoint_words:
>>          stmdb    sp!, {r3 - r9, ip}
>>
>> IE. It implies that it copies from 'from' and to 'to' in the comment,
>> or in other words copies from memory pointed to by 'R0' to memory
>> pointed to by 'R1' but in fact the implementation does the copy the
>> other way around!
>>
>> The quick and dirty fix would be to apply a (const *) cast to the 'to'
>> arguments for ARM 32. However, I think this is just too broken and
>> misleading.
>>
>> My proposal is to fix this properly and have the assembler copy the
>> correct way, and then delete the nasty conditionalisation on the
>> calls. I also propose using symbolic names 'from' and 'to' rather than
>> 'r0' and 'r1'.
>>
>> Many thanks,
>> Ed.
>>
> 
> 
> 
Not an official reviewer, but the Arm32 code looks OK.

Cheers,
Chris