8218811: replace open by os::open in hotspot coding - was : open-calls in hotspot code instead of os::open ?

Wed Feb 13 11:42:37 UTC 2019

My problem with this change are potential re entrant calls which may cause
crashes or deadlocks. In addition, there are dependencies on VM
initialization order to consider.

By wholesale exchanging open() with os::open(), you add dependencies to VM
infrastructure at a lot of places where we did have none before. VM
infrastructure which may not be reentrant and/or which may be dependent on
VM initialization.

Example: Lets say you crash in an os::open() function, enter error
reporting, which calls the same os::open() function.  Then you have a
circularity where you may crash again or lock.

Even if it works today: these things like to grow and bitrot. Today
os::open is simple, tomorrow someone else could very well have the idea of
adding hooks to os::open in the future to monitor, count, etc, and boom,
you have new dependencies.

Case in point is os::malloc() which used to be a harmless drop in malloc
replacement and now is a Moloch which calls, among other things, into NMT.

I think one has to look at every callsite in your change and determine if:

- it runs the risk of being called recursively (e.g. anything triggered
from signal handling, or a general purpose class like ostream)
- it could theoretically be called before VM initialization or argument
parsing, or after cleanup

Some examples:

--

src/hotspot/os/linux/os_linux.cpp

- _print_ascii_file() is used during error reporting.
-  os::get_core_path() is used during error reporting.

--

src/hotspot/share/runtime/arguments.cpp

Arguments::parse_vm_options_file(const char* file_name, ScopedVMInitArgs*
vm_args)

initialization dependency here: this gets called before all VM options are
parsed, therefore I would not use VM functions which may rely on VM options.

--

src/hotspot/share/utilities/ostream.cpp

 fdStream::fdStream(const char* file_name) {
-  _fd = open(file_name, O_WRONLY | O_CREAT | O_TRUNC, 0666);
+  _fd = os::open(file_name, O_WRONLY | O_CREAT | O_TRUNC, 0666);
   _need_close = true;
 }

fdStream is a general purpose class and used in a lot of places, including
os::abort and error reporting, so I would be against this change. However,
this constructor variant is called from nowhere: I would scratch this
constructor completely. That would simplify fdStream and you also can get
rid of the whole _needs_close logic inside fdStream.

Cheers, Thomas

P.S. what I actually think the VM needs, and what is missing, is the
concept of layers - some functions are more "basic" than others and should
not call upward. Examples would be logging (UL) or os::malloc(). For
instance, as it is today, UL is not usable before VM initialization, and
cannot be used within functions which are used by UL, e.g. os::malloc.

On Wed, Feb 13, 2019 at 11:35 AM Baesken, Matthias <matthias.baesken at sap.com>
wrote:

> Thanks for the feedback .
>
> Do you see any  special  issue with the os::open implementations that
> forbid usage in os::error  ?
>
> Kim what do you think ?
>
>
>
> I found  over  80  references to os::…  calls   in   vmError.cpp  so I
> think it is not generally a bad thing to do it  (however for course there
> might be functions in the os:: layer that
>
>   cause trouble in error reporting).
>
>
>
>
>
> New webrev :  (compilerWarnings_gcc.hpp   deprecation open  deprecation
>   + vmError.cpp  adjustment removed)
>
>
>
>
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8218811.1/
>
>
>
>
>
> Best regards, Matthias
>
>
>
>
>
> *From:* Thomas Stüfe <thomas.stuefe at gmail.com>
> *Sent:* Mittwoch, 13. Februar 2019 10:41
> *To:* Baesken, Matthias <matthias.baesken at sap.com>
> *Cc:* hotspot-dev at openjdk.java.net; Kim Barrett <kim.barrett at oracle.com>
> *Subject:* Re: 8218811: replace open by os::open in hotspot coding - was
> : open-calls in hotspot code instead of os::open ?
>
>
>
> Hi Matthias,
>
>
>
> not a full review but please do not use os::open in error reporting:
>
>
>
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8218811.0/src/hotspot/share/utilities/vmError.cpp.udiff.html
>
>
>
> We should use as little as possible VM functions here, since they may be
> broken and/or pull other VM dependencies which may be broken or introduce
> circularities.
>
>
>
> I am not sure if the same argument holds for other call sites as well.
>
>
>
> Best Regards, Thomas
>
>
>
>
>
>
>
> On Tue, Feb 12, 2019 at 5:19 PM Baesken, Matthias <
> matthias.baesken at sap.com> wrote:
>
> Hello,  here is a first  webrev  + bug  :
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8218811.0/
>
> https://bugs.openjdk.java.net/browse/JDK-8218811
>
> The  gcc    deprecation attribute for  open    in
> hotspot/share/utilities/compilerWarnings_gcc.hpp   is still a bit of a
> hack  ,
> Maybe you have a better idea for disabling  ( is there a good way to use a
> pragma instead of the  define ) ?
>
> Additionally I was not 100 % sure - are there maybe a few places where we
> want to stay away from os::open  for good reason ?
>
>
> Best regards, Matthias
>
>
> > -----Original Message-----
> > From: Baesken, Matthias
> > Sent: Dienstag, 12. Februar 2019 10:17
> > To: hotspot-dev at openjdk.java.net; 'Kim Barrett' <kim.barrett at oracle.com>
> > Subject: 8218811: replace open by os::open in hotspot coding - was :
> open-
> > calls in hotspot code instead of os::open ?
> >
> > Hi  Ioi / Kim  I created
> >
> > https://bugs.openjdk.java.net/browse/JDK-8218811
> >
> > 8218811:  replace open by os::open in hotspot coding
> >
> > >
> > > Not yet, but see https://bugs.openjdk.java.net/browse/JDK-8214976
> > >
> >
> > Regarding     https://bugs.openjdk.java.net/browse/JDK-8214976
> >
> >
> > > For functions which should never be called outside the implementation
> of
> > the os replacement, we can use (for example)
> > >
> > > extern "C" int vsnprintf(char*, size_t, const char*, va_list)
> > > __attribute__((__deprecated__("use os::vsnprintf")));
> > >
> > > and in the definition of os::vsnprintf, locally disable the deprecation
> > warning with the appropriate diagnostic #pragma.
> >
> > Should I add something like this for open   to  compilerWarnings.hpp  ?
> > I think if yes, I better restrict this for now to gcc  .
> >
> >
> > Best regards, Matthias
> >
> > >
> > > Message: 4
> > > Date: Thu, 7 Feb 2019 12:40:00 -0500
> > > From: Kim Barrett <kim.barrett at oracle.com>
> > > To: Ioi Lam <ioi.lam at oracle.com>
> > > Cc: hotspot-dev developers <hotspot-dev at openjdk.java.net>
> > > Subject: Re: open-calls in hotspot code instead of os::open ?
> > > Message-ID: <0FD37FD4-A478-4849-B474-A3A8CDCDD6D5 at oracle.com>
> > > Content-Type: text/plain; charset=us-ascii
> > >
> > > > On Feb 6, 2019, at 9:08 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
> > > >
> > > > I think this should be fixed.
> > >
> > > +1
> > >
> > > > BTW, is there a way to forbid all the calls to ::open()?
> > >
> > > Not yet, but see https://bugs.openjdk.java.net/browse/JDK-8214976
> > >
> > >
>
>