URGENT: Request for review: 7122222: GC log is limited to 2G for 32-bit

[Mikael Gerdin]

Dan,

On 2013-06-06 15:57, Daniel D. Daugherty wrote:
> Mikael,
>
> Sorry for top posting a reply here, but the thread below is just
> getting too muddled.
>
> I'm not quite clear on why you keep trying to distinguish between
> something being a "#define" or being a bound linkage symbol. In
> either case, the end result will be the same. The ostream.o module
> will call foo64() versions of functions instead of foo() versions.
>
> Consider this:
>
> - ostream.cpp can include a great many .hpp files or not
> - those .hpp files may have functions affected by _FILE_OFFSET_BITS
>    or not
> - the only functions in any of the .hpp files that matter to ostream.o
>    are the ones that code in ostream.o actually calls
> - if ostream.o doesn't attempt to use/bind to a function then it
>    doesn't matter whether it has been #define'ed or rebound to a new
>    name
>
> What really matters here is that ostream.o will call foo64() and some
> other object file, bar.o, will call foo(). If ostream.o and bar.o are
> operating on the same FILE or file, then we will have problems. How
> we got there (#define or binding) is not really the critical point.

I agree with your summary.
I don't plan to push my opinions on this any further.

/Mikael

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