On Fri, 25 Sep 2020 05:46:08 GMT, Kim Barrett <kbarr...@openjdk.org> wrote:
>> Please review this small patch to enable the OSX build using Xcode 12.0. >> >> Thanks, >> Paul > > 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. I checked Kim Barrett's proposals (1) and (2) on my machine (MacOS 10.15, Xcode 12.0). Both proposals make the warning go away. Another note, actually, it's more like a question: Has anyone had an eye on what happens in initialize_shared_locs(relocInfo* buf, int length)? To my understanding, "buf", which is passed in as "locs_buf", is stored into CodeBuffer fields. Is that a good idea? "locs_buf" points into the stack. This pointer becomes invalid at the end of the "locs_buf" scope. Did I get something wrong here? ------------- PR: https://git.openjdk.java.net/jdk/pull/348