Thanks for the reviews/discussion, and apologies for the delayed reply: I've
been OOTO.
Kim is correct, initialize_shared_locs specifically adjusts the alignment of
its buffer argument, which is why short works. char would work as well, but
short happens to be the size of a relocInfo. Maybe the author of the other use
in sharedRuntime.cpp at line 2682 used short to remind of that.
Code that calls initialize_shared_locs is inconsistent even within itself.
E.g., in c1_Compilation.cpp, we have
int locs_buffer_size = 20 * (relocInfo::length_limit + sizeof(relocInfo));
char* locs_buffer = NEW_RESOURCE_ARRAY(char, locs_buffer_size);
code->insts()->initialize_shared_locs((relocInfo*)locs_buffer,
locs_buffer_size / sizeof(relocInfo));
relocInfo::length_limit is a count of shorts, while sizeof(relocInfo) is a
count of chars. The units aren’t the same but are added together as if they
were. relocInfo::length_limit is supposed to be the maximum size of a
compressed relocation record, so why add sizeof(relocInfo)?
And then in sharedRuntime.cpp, we have two places. The first:
short buffer_locs[20];
buffer.insts()->initialize_shared_locs((relocInfo*)buffer_locs,
sizeof(buffer_locs)/sizeof(relocInfo));
relocInfo::length_limit is 15 on a 64-bit machine, so with a buffer of 20
shorts, alignment in initialize_shared_locs might take up to of 3 more, which
is uncomfortably close to 20 afaic. And, if you add sizeof(relocInfo) as
happens in c1_Compilation.cpp, you're bang on at 20. The unstated assumption
seems to be that only a single relocation record will be needed.
The second:
double locs_buf[20];
buffer.insts()->initialize_shared_locs((relocInfo*)locs_buf,
sizeof(locs_buf) / sizeof(relocInfo));
This at allocates a buffer that will hold 5 compressed relocation records with
10 bytes left over, and guarantees 8 byte alignment. Perhaps when it was
written, initialize_shared_locs didn't align its buffer argument address. And,
there's that sizeof(relocInfo) padding again: 2 extra bytes per relocation
record.
Anyway, I just wanted to make the compiler warning go away, not figure out why
the code is the way it is. Matthias and Kim would like to separate out the
CSystemColors.m patch into a separate bug, which is fine by me (see separate
email).
So, for the sharedRuntime patch, would it be ok to just use
short locs_buf[84];
to account for possible alignment in initialize_shared_locs? I'm using 84
because 80 is exactly 5 records plus 5 sizeof(relocInfo)s, allowing for
alignment adds 3, and rounding the total array size up to a multiple of 8 adds
1.
Thanks,
Paul
On 9/29/20, 5:03 AM, "2d-dev on behalf of Kim Barrett"
<2d-dev-r...@openjdk.java.net on behalf of kim.barr...@oracle.com> wrote:
> On Sep 29, 2020, at 3:59 AM, Matthias Baesken <mbaes...@openjdk.java.net>
wrote:
>
> On Fri, 25 Sep 2020 02:23:07 GMT, David Holmes <dhol...@openjdk.org>
wrote:
>
>>> Please review this small patch to enable the OSX build using Xcode 12.0.
>>>
>>> Thanks,
>>> Paul
>>
>> src/hotspot/share/runtime/sharedRuntime.cpp line 2851:
>>
>>> 2849: if (buf != NULL) {
>>> 2850: CodeBuffer buffer(buf);
>>> 2851: short locs_buf[80];
>>
>> This code is just weird. Why is the locs_buf array not declared to be
the desired type? If the compiler rejects double
>> because it isn't relocInfo* then why does it accept short? And if it
accepts short will it accept int or long long or
>> int64_t? Using int64_t would be a clearer change. Though semantically
this code is awful. :( Should it be intptr_t ?
>
> Currently we have in the existing code various kind of buffers passed
into initialize_shared_locs that compile nicely
> on all supported compilers and on Xcode 12 as well ,see
>
> c1_Compilation.cpp
>
> 326 char* locs_buffer = NEW_RESOURCE_ARRAY(char, locs_buffer_size);
> 327 code->insts()->initialize_shared_locs((relocInfo*)locs_buffer,
>
> sharedRuntime.cpp
>
> 2681 CodeBuffer buffer(buf);
> 2682 short buffer_locs[20];
> 2683 buffer.insts()->initialize_shared_locs((relocInfo*)buffer_locs,
> 2684
sizeof(buffer_locs)/sizeof(relocInfo));
>
> So probably using short would be consistent to what we do already in the
coding at another place (looking into
> relocInfo it would be probably better to use unsigned short or to
typedef unsigned short in the relocInfo class
> and use the typedef).
That’s *not* consistent, because of buffer alignment. The call to
NEW_RESOURCE_ARRAY is going
to return a pointer to memory that is 2*word aligned. (Interesting,
possibly a 32/64 bit “bug” here.)
The existing code in sharedRuntime.cpp is allocating double-aligned.
iniitalize_shared_locs wants a
HeapWordSize-aligned buffer, and adjusts the pointer it uses accordingly.
(I think with existing code
it could just make the alignment of the buffer a precondition, but I
haven’t looked at all callers.)
Changing the declaration in sharedRuntime.cpp to short[] reduces the
alignment requirement for the
array, possibly requiring alignment adjustment (and hence size reduction)
by initialize_shared_locs.
Since initialize_shared_locs specifically adjusts the alignment, some
downstream use of the buffer
probably actually cares.
>
> -------------
>
> PR: https://git.openjdk.java.net/jdk/pull/348
