<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Mikael,<br>
<br>
Let me explain the changeset first before I go into your comments.<br>
<br>
(1) make/linux/makefiles/vm.make<br>
<br>
<meta charset="utf-8">
# Large File Support <br>
ifneq ($(LP64), 1) <br>
CXXFLAGS/ostream.o += -D_FILE_OFFSET_BITS=64 <br>
endif # ifneq ($(LP64), 1)<br>
<br>
For "Linux" code, set _FILE_OFFSET_BITS=64 "only" to ostream.o (i.e.
ostream.{c,h}pp) in order to bind all foo() functions to foo64()
functions when linking libraries. This will enable functions in
ostream.{c,h}pp to deal with large files while all other files won't
have the ability. And that's what we want at this point.<br>
<br>
(2) make/solaris/makefiles/vm.make<br>
<br>
# Large File Support <br>
ifneq ($(LP64), 1) <br>
CXXFLAGS/ostream.o += -D_FILE_OFFSET_BITS=64 <br>
endif # ifneq ($(LP64), 1)<br>
<br>
Similarly, for "Solaris" code, set _FILE_OFFSET_BITS=64 "only" to
ostream.o (i.e. ostream.{c,h}pp) in order to bind all foo()
functions to foo64() functions when linking libraries. This will
enable functions in ostream.{c,h}pp to deal with large files while
all other files won't have the ability.<br>
<br>
(1) & (2) altogether set _FILE_OFFSET_BITS=64 "only" to
ostream.o in "Linux" and "Solaris" code base and have not changed
Windows and BSD code, as they can handle large files currently. No
need to change.<br>
<br>
Remember the current implementation has a limited affected range
(i.e. ostream.o for Linux and Solaris). No more no less.<br>
<br>
(3) src/os/solaris/vm/os_solaris.inline.hpp<br>
<br>
<meta charset="utf-8">
<pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; white-space: pre-wrap;">#if defined(_LP64) || defined(_GNU_SOURCE) || _FILE_OFFSET_BITS==64
dirent* p;
int status;
if((status = ::readdir_r(dirp, dbuf, &p)) != 0) {
errno = status;
return NULL;
} else
return p;
#else // defined(_LP64) || defined(_GNU_SOURCE) || _FILE_OFFSET_BITS==64
return ::readdir_r(dirp, dbuf);
#endif // defined(_LP64) || defined(_GNU_SOURCE) || _FILE_OFFSET_BITS==64</pre>
<br>
This piece of code handles some exception I need to fix. It is not
what I want to change but rather what I have to change. <br>
<br>
Please see inline.<br>
<br>
Thanks.<br>
Tao<br>
<br>
<br>
On 6/5/13 9:02 AM, Mikael Gerdin wrote:
<blockquote cite="mid:51AF6126.5050106@oracle.com" type="cite">Tao,
<br>
<br>
On 2013-06-05 17:48, Tao Mao wrote:
<br>
<blockquote type="cite">Thank you for comments. One thing I want
to point out is that the
<br>
current change has not touched on Windows code.
<br>
<br>
Please see inline.
<br>
<br>
Tao
<br>
<br>
On 6/5/13 1:19 AM, Mikael Gerdin wrote:
<br>
<blockquote type="cite">
<br>
<br>
On 2013-06-05 02:21, Daniel D. Daugherty wrote:
<br>
<blockquote type="cite">OK, based on the largefiles.pdf
write-up, your use of
<br>
_FILE_OFFSET_BITS=64
<br>
is going to cause ostream.o to bind to various 64-bit
versions of some
<br>
functions. Based on my very hazy memory of my days in file
system code,
<br>
this binding is going to affect ostream.o only unless
ostream.o is
<br>
writing to some file with the foo64() function and some
other code is
<br>
also writing to the same file at the same time with foo().
However,
<br>
if we
<br>
have two different functions writing to the same open file
at the same
<br>
time, then we have bigger issues. :-)
<br>
<br>
I'm good with these changes now. I agree that solving the
problem of
<br>
setting _FILE_OFFSET_BITS=64 for the entire VM build doesn't
have to
<br>
solved right now.
<br>
</blockquote>
<br>
I think this change is really scary, setting
_FILE_OFFSET_BITS=64 when
<br>
compiling ostream.cpp will effectively cause the headers to
swap out
<br>
the implementations of the f[open,tell,seek] to 64 bit
versions for
<br>
all headers that are included and inlined in ostream.cpp.
<br>
Other parts of the code using the same headers will see
different
<br>
versions of the functions with different parameters due to
off_t
<br>
changing sizes.
<br>
</blockquote>
The change is currently effective for Linux and Solaris if you
look at
<br>
the file directories. Nothing changed for Windows and BSD, as
they don't
<br>
need such change.
<br>
</blockquote>
<br>
Right.
<br>
But if you use my suggested approach you would need to change
calls to fopen() in ostream.cpp to fopen_pd where
<br>
<br>
if (linux || solaris) && 32bit
<br>
#define fopen_pd fopen64
<br>
else
<br>
#define fopen_pd fopen
<br>
</blockquote>
I saw what you suggested. <br>
<br>
But, what's the benefit of doing so, or, what's the severe problem
about the current changeset? I don't see that one's better than
another.<br>
<br>
<blockquote cite="mid:51AF6126.5050106@oracle.com" type="cite">
<br>
<blockquote type="cite">
<blockquote type="cite">
<br>
I think that what we should do is to use the "Transitional
compilation
<br>
environment" detailed in §2.6.2 in largefiles.pdf and change
the calls
<br>
in ostream.cpp to use the appropriate f[open,tell,seek]64
functions
<br>
directly. I feel this is especially important at this late
stage in
<br>
the release to make an explicit change instead of setting a
#define
<br>
which has propagating side-effects.
<br>
</blockquote>
How do you see "propagating side-effects" and to where?
<br>
</blockquote>
<br>
_FILE_OFFSET_BITS=64 changes the definition of fopen for every
file including stdio.h.
<br>
</blockquote>
The current implementation has a limited affected range (i.e.
ostream.o for Linux and Solaris). No more no less.<br>
<br>
How do you see this flag will affect every file?<br>
<blockquote cite="mid:51AF6126.5050106@oracle.com" type="cite">
<br>
It's confusing when a call to "fopen()" in one file calls the 64
bit version and in other files it doesn't.
<br>
<br>
How will this work with precompiled headers? Which version of
fopen will be in the precompiled header file?
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">
<br>
As Tao mentioned this will require us to handle the fact that
there is
<br>
no fopen64() call on Windows, that the CRT fopen() already
seems to
<br>
handle large files and that ftell64() and fseek64() have
slightly
<br>
different names on Windows. I don't think this is a large
hurdle and I
<br>
think we know how to solve this problem.
<br>
</blockquote>
As I said, nothing was changed for Windows code.
<br>
</blockquote>
<br>
No, but to be consistent you'd need to update the ftell* fseek* to
use 64 bit versions, right?
<br>
<br>
/Mikael
<br>
<br>
<blockquote type="cite">
<blockquote type="cite">
<br>
/Mikael
<br>
<br>
<blockquote type="cite">
<br>
Dan
<br>
<br>
<br>
On 6/4/13 6:06 PM, Tao Mao wrote:
<br>
<blockquote type="cite">Thank you for review, Dan.
<br>
<br>
I'll try to answer as much as I can. Please see inline.
<br>
<br>
Thanks.
<br>
Tao
<br>
<br>
On 6/4/13 4:35 PM, Daniel D. Daugherty wrote:
<br>
<blockquote type="cite">>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tamao/7122222/webrev.00/">http://cr.openjdk.java.net/~tamao/7122222/webrev.00/</a>
<br>
<br>
Tao,
<br>
<br>
I think the lack of response to this review request is
the absolutely
<br>
strange nature of these changes. And I thought I put out
some weird
<br>
code reviews... :-)
<br>
<br>
make/linux/makefiles/vm.make
<br>
Build ostream.o with _FILE_OFFSET_BITS==64 on Linux.
Nothing
<br>
obvious in this webrev about what this will mean so
I took a
<br>
look at src/share/vm/utilities/ostream.{c,h}pp and I
see no
<br>
use of _FILE_OFFSET_BITS in either of those source
files.
<br>
<br>
Must be in the source files somewhere, but I can't
find any
<br>
use of _FILE_OFFSET_BITS in the entire hotspot
source base.
<br>
<br>
make/solaris/makefiles/vm.make
<br>
Build ostream.o with _FILE_OFFSET_BITS==64 on Solaris.
<br>
<br>
OK, I looked for _FILE_OFFSET_BITS in /usr/include
on my
<br>
Solaris box. Lots of references, but nothing that
helps me
<br>
understand what you're doing here.
<br>
src/os/solaris/vm/os_solaris.inline.hpp
<br>
The addition of _FILE_OFFSET_BITS==64 means that the
<br>
os::readdir() function will use the safer,
multi-threaded
<br>
version of readdir_r(). Seems fine to me.
<br>
<br>
Here's what I need to know:
<br>
<br>
- what effect does _FILE_OFFSET_BITS have on building
ostream.{c,h}pp?
<br>
</blockquote>
_FILE_OFFSET_BITS is set to be picked by c++ compiler.
<br>
<br>
For why we need to set _FILE_OFFSET_BITS==64 in this case,
please
<br>
refer to the following document
<br>
<br>
This Sun White Paper
<br>
(<a class="moz-txt-link-freetext" href="http://unix.business.utah.edu/doc/os/solaris/misc/largefiles.pdf">http://unix.business.utah.edu/doc/os/solaris/misc/largefiles.pdf</a>)
<br>
summarizes the usage of the flags on solaris (page
"5-26"). And, it
<br>
should apply to Linux the same way as was agreed across
platforms
<br>
(<a class="moz-txt-link-freetext" href="http://linuxmafia.com/faq/VALinux-kb/2gb-filesize-limit.html">http://linuxmafia.com/faq/VALinux-kb/2gb-filesize-limit.html</a>).
<br>
<br>
<blockquote type="cite">- if ostream.o has one idea about
the value of _FILE_OFFSET_BITS
<br>
what happens if another part of the VM has a different
idea about
<br>
the value of _FILE_OFFSET_BITS?
<br>
</blockquote>
_FILE_OFFSET_BITS is not set for other particular effects,
but for
<br>
extending the ability to deal with large files in
ostream.{c,h}pp. So,
<br>
if other files have a different idea about
_FILE_OFFSET_BITS, they
<br>
can't deal with large files. No more no less.
<br>
<br>
<blockquote type="cite">
<br>
I saw this in the post to the Runtime alias:
<br>
<br>
> Included runtime dev to see whether they have some
idea to handle
<br>
> the compilation choices.
<br>
<br>
And I still have no idea what you're asking here? What
compilation
<br>
choices? Are you asking about your Makefile changes? Are
asking
<br>
about defining _FILE_OFFSET_BITS for the entire build
instead of
<br>
just one object (ostream.o)? Are you worried that this
VM is going
<br>
to have mis-matched pieces and be unstable?
<br>
</blockquote>
"Are asking about defining _FILE_OFFSET_BITS for the
entire build
<br>
instead of just one object (ostream.o)?" is my main
question I
<br>
originally tried to ask.
<br>
<br>
<blockquote type="cite">
<br>
So I reviewed it, but I definitely can't approve it
without more
<br>
info. I realize that you're up against the RDP2 limit,
but this
<br>
change has too many open questions (for now)...
<br>
<br>
BTW, it is not at all clear whether Win32 will be able
to write a 2GB+
<br>
GC log or not. The conversation below didn't help me at
all.
<br>
</blockquote>
I used a jdk7 (just any) to successfully generate a log
file larger
<br>
than 4GB. So, it shouldn't be a problem for Win32.
<br>
<blockquote type="cite">
<br>
Dan
<br>
<br>
<br>
<br>
On 6/4/13 5:03 PM, Tao Mao wrote:
<br>
<blockquote type="cite">Since the changeset touched
makefiles, I've included
<br>
<a class="moz-txt-link-abbreviated" href="mailto:build-dev@openjdk.java.net">build-dev@openjdk.java.net</a> .
<br>
<br>
I need to push the hsx24 bug asap. Please review it.
<br>
<br>
Thanks.
<br>
Tao
<br>
<br>
On 6/4/13 2:37 PM, Tao Mao wrote:
<br>
<blockquote type="cite">Hi all,
<br>
<br>
Need reviews to catch RDP2.
<br>
<br>
The current webrev is a working solution to all
platforms, Linux,
<br>
Windows, and Solaris.
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tamao/7122222/webrev.00/">http://cr.openjdk.java.net/~tamao/7122222/webrev.00/</a>
<br>
<br>
Thanks.
<br>
Tao
<br>
<br>
On 5/30/13 10:21 AM, Tao Mao wrote:
<br>
<blockquote type="cite">Included runtime dev to see
whether they have some idea to handle
<br>
the compilation choices.
<br>
<br>
For now, it's been verified that the fix is
functionally
<br>
sufficient.
<br>
<br>
Thanks.
<br>
Tao
<br>
<br>
On 5/29/13 5:27 PM, Tao Mao wrote:
<br>
<blockquote type="cite">Thank you, Mikael.
<br>
<br>
Please see inline.
<br>
<br>
Reviewers, please review it based on the
following new
<br>
observation.
<br>
<br>
Tao
<br>
<br>
On 5/27/13 2:05 AM, Mikael Gerdin wrote:
<br>
<blockquote type="cite">Tao,
<br>
<br>
On 2013-05-25 02:19, Tao Mao wrote:
<br>
<blockquote type="cite">7ux bug
<br>
<br>
webrev:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tamao/7122222/webrev.00/">http://cr.openjdk.java.net/~tamao/7122222/webrev.00/</a>
<br>
<br>
changeset:
<br>
(1) make -D_FILE_OFFSET_BITS=64 only
available to generating
<br>
ostream.o
<br>
<br>
Why conservative rather than making
-D_FILE_OFFSET_BITS=64
<br>
globally
<br>
applicable?
<br>
<br>
Global setting of -D_FILE_OFFSET_BITS=64 on
linux works fine;
<br>
however,
<br>
there are at least five code conflicts if
introducing the flag
<br>
globally
<br>
to Solaris.
<br>
<br>
One was resolved as in
os_solaris.inline.hpp, but the rest four
<br>
files
<br>
had conflicts deep in c library. Even if
they are excluded from
<br>
setting
<br>
-D_FILE_OFFSET_BITS=64, the compiled VM is
corrupted.
<br>
<br>
(2) For now, no Windows solution.
<br>
I haven't found any clean solution for
solving this problem on
<br>
Windows.
<br>
</blockquote>
<br>
This seems like an insufficient fix if you
can't make it work on
<br>
all platforms. I tried building with
"-D_LARGEFILE64_SOURCE
<br>
-D_FILE_OFFSET_BITS=64" ons Solaris and hit an
#error in
<br>
libelf.h saying it wasn't supported so I
understand your problem
<br>
there.
<br>
</blockquote>
Yes, that's my grief :( you touched them, a
bunch of them. That's
<br>
why I chose to apply the flag only to the files
(ostream.cpp and
<br>
ostream.hpp) I want the effect.
<br>
<blockquote type="cite">
<br>
Instead I suggest that you use the
compatibility API described
<br>
in lf64(5) on Solaris. This API consists of
fopen64, ftell64 and
<br>
friends and is exposed when
"-D_LARGEFILE64_SOURCE" is set.
<br>
<br>
The same "-D_LARGEFILE64_SOURCE" is available
on Linux and has
<br>
the added advantage of not changing any
existing symbols and
<br>
therefore we can set the define for all files
instead of just
<br>
ostream
<br>
<br>
This approach has the added advantage that it
more closely
<br>
resembles the changes which will be needed for
Windows anyway.
<br>
Those changes would consist of changing calls
to ftell/fseek to
<br>
64-bit versions and changing fopen to fopen64
on Solaris/Linux.
<br>
</blockquote>
Both ways have pros and cons. The current
implementation excludes
<br>
the usage of fopen64, providing portability
(since there's no
<br>
fopen64 for Windows). Meanwhile, I understand
your suggestion
<br>
provides other benefits.
<br>
<br>
This Sun White Paper
<br>
(<a class="moz-txt-link-freetext" href="http://unix.business.utah.edu/doc/os/solaris/misc/largefiles.pdf">http://unix.business.utah.edu/doc/os/solaris/misc/largefiles.pdf</a>)
<br>
summarizes
<br>
the usage of the flags on solaris (Page 5-26).
And, it should
<br>
apply to Linux the same way as was agreed across
platforms.
<br>
<br>
<blockquote type="cite">
<br>
Since there is no fopen64 on Windows it seems
that the default
<br>
fopen already supports large files.
<br>
</blockquote>
I tested, and you are correct that the 32-bit VM
on Windows can
<br>
write beyond 2GB (and beyond 4GB). Thank you,
it's solved "half
<br>
of my problem" :)
<br>
<blockquote type="cite">
<br>
/Mikael
<br>
<br>
<blockquote type="cite">
<br>
test:
<br>
(1) Ability to write over 2g file for 32-bit
builds were
<br>
verified on the
<br>
following configurations.
<br>
<br>
Linux * i586
<br>
Solaris * i586
<br>
Solaris * sparc
<br>
<br>
(2) Need a JPRT test for sanity check.
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</body>
</html>