> On Sep 25, 2020, at 1:49 AM, Kim Barrett <kbarr...@openjdk.java.net> wrote: > > On Thu, 24 Sep 2020 21:28:01 GMT, Paul Hohensee <p...@openjdk.org> wrote: > >> Please review this small patch to enable the OSX build using Xcode 12.0. >> >> Thanks, >> Paul
[I tried submitting this comment through the github UI and somehow managed to lose all the context in the process. In case it’s not (eventually) obvious, this is about the change to src/hotspot/share/runtime/sharedRuntime.cpp. Still figuring out github…] > No, don't do this. In the original, double is used to obtain the desired > alignmnent. Changing the element type to short reduces the alignment > requirement for the variable. initialize_shared_locs will then adjust the > alignment, potentially shrinking the effective array size. So this is a > real change that I think shouldn't be made. > > I think changing the declaration for locs_buf to any of the following gives > equivalent behavior to the current code. I don't know whether any of them > will trigger the new clang warning though. I don't have access to that > version right now, and the documentation for the warning is useless (like so > much of clang's warning options documentation). > > (1) alignas(double) char locs_buf[20 * sizeof(double)]; > (but alignas is not yet in the "permitted features" list in the Style Guide: > https://bugs.openjdk.java.net/browse/JDK-8252584) > > (2) union { char locs_buf[20 * sizeof(double)]; double align; }; > > (3) std::aligned_storage_t<20 * sizeof(double)> locs_buf; > and change (relocInfo*)locs_buf => (relocInfo*)&locs_buf > and #include <type_traits> > This has the benefit of being explicit that we're just stack allocating a > block of storage. > > I'll make a wild guess that (1) and (2) will still warn, though char arrays > are somewhat special in the language so maybe they won't. I'm pretty sure > (3) should do the trick. > > ------------- > > Changes requested by kbarrett (Reviewer). > > PR: https://git.openjdk.java.net/jdk/pull/348
