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 fopenI 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. /MikaelIt'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/MikaelDan 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@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" :)/Mikaeltest: (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.
