RFR: 8263185: Mallinfo deprecated in glibc 2.33 [v2]

[David Holmes]

Hi Christoph,

Is there not a pragma we can use to suppress the warning? Or can we 
disable that particular warning in the build? We will have to deal with 
this more completely one day, but hopefully by then we can just build 
and run for the mallinfo2 API.

Cheers,
David

On 15/03/2021 10:13 pm, Christoph Göttschkes wrote:
> On Sun, 14 Mar 2021 05:20:05 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:
> 
>>> Christoph Göttschkes has updated the pull request incrementally with one additional commit since the last revision:
>>>
>>>    Fixes sign extension regression and resolves mallinfo using dlsym.
>>
>> Hi Christoph,
>>
>> this looks fine in principle, but could you please move the symbol resolution to the init phase? There are plenty of examples, e.g. `os::Linux::sched_getcpu_init`, `os::Linux::libnuma_init`. (They are all exposed via the os::Linux namespace which I personally don't like but when in Rome...)
>>
>> The main reason for this is that `os::Linux::print_process_memory_information` may be called during error reporting, in the context of a crash, and I don't want to call as little system APIs as possible there.
>>
>> BTW, you can also test this with -XX:ErrorHandlerTest=*number*, with e.g. 14 I believe giving you a real SEGV. The information should be printed correctly as part of the hs-err file.
>>
>> Also please use CAST_TO_FN_PTR instead of manually casting. In general, its good to keep to the existing style wrt to C++ features. For one, we have a list of allowed and disallowed list of C++ features, which is not immediately clear (see hotspot style guide in top level doc folder). And I personally don't like mixing styles, better to change code to more modern C++ syntax in specific RFEs if that is wanted. Also makes backporting functional changes easier.
>>
>> Thanks, Thomas
> 
> Hi Thomas,
> 
>> Also please use CAST_TO_FN_PTR instead of manually casting.
> 
> Sure thing, changed in the last commit.
> 
> About moving the symbol resolution. Before changing this, I would like to get your feedback, because the reason I didn't do it where you are suggesting was the amount of GLIBC version dependent code I would introduce. If I move the the resolution (and keep the current logic), I would add 3 more `ifdefs` which check the glibc version:
> 
> 1. Delcaration of the two function pointers
> 2. Initialization of the two function pointer
> 3. Symbol resolution
> 
> This seemed a lot of dependent code, all of which need two `#if`:
> 
> #ifdef GLIBC
> // declare (os_linux.hpp)  / intialize (os_linux.cpp) / resolve (os_linux.cpp os::init) mallinfo function pointer
> #if __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 33)
> // declare / intialize / resolve mallinfo2 function pointer
> #endif
> #endif
> 
> We always need two pointers, since we need to know which one we are calling because of the return type. We could only have one and do something like the following, but then we loose the ability to call `mallinfo`, if the JVM has been linked against glibc >= 2.33:
> 
> #ifdef GLIBC
> #if __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 33)
> // declare / intialize / resolve mallinfo2 function pointer
> #else
> // declare / intialize / resolve mallinfo function pointer
> #endif
> #endif
> 
> I wanted to avoid that, that's why I did the resolution where it is right now.
> 
> We could also ignore `mallinfo2` for the moment, and always call `mallinfo`, regardless of the glibc version. I think the important part right now is, two get rid of the compile time warning (by resolving `mallinfo` at runtime using `dlsym`), not enhancing the error reporting.
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/2964
>