[OpenJDK 2D-Dev] RFR: 8253375: OSX build fails with Xcode 12.0 (12A7209)

[Hohensee, Paul]

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-retn at openjdk.java.net on behalf of kim.barrett at oracle.com> wrote:

    > On Sep 29, 2020, at 3:59 AM, Matthias Baesken <mbaesken at openjdk.java.net> wrote:
    >
    > On Fri, 25 Sep 2020 02:23:07 GMT, David Holmes <dholmes at 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