On Wed, 17 Apr 2024 11:52:42 GMT, Julian Waters <jwat...@openjdk.org> wrote:
>> @magicus @TheShermanTanker @TheRealMDoerr @kimbarrett >> Let me summarize the choices we have and ask for your vote. >> Julian dislikes the `-Dalloca'(size)'=__builtin_alloca'(size)'` in >> `flags-cflags.m4` I introduced to get rid of >> >> #if defined(_AIX) >> #include <alloca.h> >> #endif >> >> in `globalDefinitions_gcc.hpp`. >> >> We have three possible solutions >> >> 1. Reintroduce >> >> #if defined(_AIX) >> #include <alloca.h> >> #endif >> >> in `globalDefinitions_gcc.hpp`. >> >> 2. Unconditionally introduce only `#include <alloca.h>` in >> `globalDefinitions_gcc.hpp`. This should work for all platforms using this >> header including the unofficial Windows/gcc Port, although only AIX needs >> it. >> >> 3. Add >> >> #if defined(_AIX) >> #include <alloca.h> >> #endif >> >> to the sources using alloca(). These are >> /hotspot/share/runtime/os.cpp >> /hotspot/share/runtime/javaThread.cpp >> /hotspot/share/utilities/vmError.cpp >> Here we need the AIX condition, because otherwise the classic Windows Build >> (NTAMD64) fails. >> >> I will implement the solution with the most likes and having no dislike. > > I don't mind all 3, though I certainly prefer 1 and 3 over 2 (The way I see > it, the AIX macro check is more of a message to the programmer than it is > important to the compiler, so I prefer the options that have it. However, I > also don't mind if we were to go the way of option 2, this is more of a > preference thing). The fact that only 3 files need it is also surprising to > me, and makes option 3 seem like a good fit (Again, personal preference) > > Magnus and Kim, what do you guys think? If there are just 3 files using alloca, I strongly prefer solution 3. I think solution 1 has already been rejected by Kim. (Also, for the record, it was me, not Julian, who expressed dislike about the `-Dalloca'(size)'=__builtin_alloca'(size)'` change) ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18536#discussion_r1568754458
