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

Thu Feb 14 10:07:34 UTC 2019

Hi Matthias,

fdStream: can you pls also remove:

fdStream::fdStream(const char* file_name)

bool _need_close;

and the close() call in the constructor?

--

CompileLog::finish_log_on_error() is also used during error reporting (see
comment above the function).

---

src/hotspot/share/utilities/compilerWarnings_gcc.hpp is a whitespace only
change.

--

Skimmed the rest and it seems ok.

Thanks Thomas

On Thu, Feb 14, 2019 at 9:49 AM Baesken, Matthias <matthias.baesken at sap.com>
wrote:

>
>
> New webrev :
>
>
>
> http://cr.openjdk.java.net/~mbaesken/webrevs/8218811.2/
>
>
>
>    - Removed my os::open  changes from os_linux.cpp  and  vmError.cpp
>    because of Thomas concerns
>
> -          Removed  fdStream::fdStream(const char* file_name) { … }  from ostream.cpp ;   I stayed away from any more “cleanups” to   ostream , could be done in a separate change in case it is wanted
>
>
>
>
>
>
>
> Best regards, Matthias
>
>
>
>
>
> *From:* Thomas Stüfe <thomas.stuefe at gmail.com>
> *Sent:* Mittwoch, 13. Februar 2019 17:42
> *To:* Baesken, Matthias <matthias.baesken at sap.com>
> *Cc:* Kim Barrett <kim.barrett at oracle.com>; ioi.lam at oracle.com;
> hotspot-dev at openjdk.java.net
> *Subject:* Re: 8218811: replace open by os::open in hotspot coding - was
> : open-calls in hotspot code instead of os::open ?
>
>
>
>
>
> Hi Matthias,
>
>
>
> On Wed, Feb 13, 2019 at 4:00 PM Baesken, Matthias <
> matthias.baesken at sap.com> wrote:
>
> Hi Thomas ,   regarding  os_linux.cpp , vmError.cpp    and
> arguments.cpp   I could remove the calls to os:::open  ( my first webrev
> did not touch  os_linux.cpp  because I thought there might be
>
> reasons to omit using it there ).
>
>
>
>
>
> makes sense.
>
>
>
> Additionally I’d  remove
>
>
>
> fdStream::fdStream(const char* file_name) {
>
>
>
> Is everyone fine with this ?
>
>
>
> Yes, please, and you could then slim down the logic inside fdStream
> somewhat. Currently it has those two modes, one where he owns the file, one
> where not. Since we remove the former case, you can remove _needs_close and
> the close() call in the destructor.
>
>
>
> Thank you,
>
>
>
> 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.
>
>
>
> I’d even like to have a very basic layer I could reuse in the native  jdk
> libs too  ��  (separate lib).
>
> However this is out of scope of this change .
>
>
>
> Best regards, Matthias
>
>
>
>
>
> *From:* Thomas Stüfe <thomas.stuefe at gmail.com>
> *Sent:* Mittwoch, 13. Februar 2019 12:43
> *To:* Baesken, Matthias <matthias.baesken at sap.com>; Kim Barrett <
> kim.barrett at oracle.com>
> *Cc:* hotspot-dev at openjdk.java.net
> *Subject:* Re: 8218811: replace open by os::open in hotspot coding - was
> : open-calls in hotspot code instead of os::open ?
>
>
>
>
>
> 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
> > >
> > >
>
>