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

Thu Feb 14 15:44:04 UTC 2019

Hello, here is the updated  webrev :

http://cr.openjdk.java.net/~mbaesken/webrevs/8218811.4/

Best regards, Matthias

From: Baesken, Matthias
Sent: Donnerstag, 14. Februar 2019 12:43
To: 'Thomas Stüfe' <thomas.stuefe at gmail.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 ?

>fdStream::fdStream(const char* file_name)
>
>bool _need_close;
>
>and the close() call in the constructor?

Hi, this has nothing to do with this change.
I have very bad experience with bringing in unrelated cleanups into  a change my request after request etc.

Will remove  the os::open  from   CompileLog::finish_log_on_error()     and the whitespace stuff.

Thanks, Matthias

From: Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>
Sent: Donnerstag, 14. Februar 2019 11:08
To: Baesken, Matthias <matthias.baesken at sap.com<mailto:matthias.baesken at sap.com>>
Cc: Kim Barrett <kim.barrett at oracle.com<mailto:kim.barrett at oracle.com>>; ioi.lam at oracle.com<mailto:ioi.lam at oracle.com>; hotspot-dev at openjdk.java.net<mailto: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,

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<mailto: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<mailto:thomas.stuefe at gmail.com>>
Sent: Mittwoch, 13. Februar 2019 17:42
To: Baesken, Matthias <matthias.baesken at sap.com<mailto:matthias.baesken at sap.com>>
Cc: Kim Barrett <kim.barrett at oracle.com<mailto:kim.barrett at oracle.com>>; ioi.lam at oracle.com<mailto:ioi.lam at oracle.com>; hotspot-dev at openjdk.java.net<mailto: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<mailto: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<mailto:thomas.stuefe at gmail.com>>
Sent: Mittwoch, 13. Februar 2019 12:43
To: Baesken, Matthias <matthias.baesken at sap.com<mailto:matthias.baesken at sap.com>>; Kim Barrett <kim.barrett at oracle.com<mailto:kim.barrett at oracle.com>>
Cc: hotspot-dev at openjdk.java.net<mailto: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<mailto: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<mailto:thomas.stuefe at gmail.com>>
Sent: Mittwoch, 13. Februar 2019 10:41
To: Baesken, Matthias <matthias.baesken at sap.com<mailto:matthias.baesken at sap.com>>
Cc: hotspot-dev at openjdk.java.net<mailto:hotspot-dev at openjdk.java.net>; Kim Barrett <kim.barrett at oracle.com<mailto: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<mailto: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<mailto:hotspot-dev at openjdk.java.net>; 'Kim Barrett' <kim.barrett at oracle.com<mailto: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<mailto:kim.barrett at oracle.com>>
> > To: Ioi Lam <ioi.lam at oracle.com<mailto:ioi.lam at oracle.com>>
> > Cc: hotspot-dev developers <hotspot-dev at openjdk.java.net<mailto: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<mailto: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<mailto: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
> >
> >