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

[Thomas Stüfe]

Hi David,

On Mon, Mar 15, 2021 at 1:28 PM David Holmes <david.holmes at oracle.com>
wrote:

> 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
>

I disagree with that. mallinfo2 has the obvious advantage of giving us
correct results where legacy mallinfo would report garbage. This does not
sound exciting until you are in a support situation where all you get is an
hs-err file.

Since we will support glibc's lacking mallinfo2 for a very long time to
come, I'd prefer we take Christoph's enhancement. If done well, this patch
does not have to large or complex.

Cheers, Thomas


>
> 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
> >
>