Hi Dan,
You got that right. :)
Thanks for the review!
/Erik
> On 5 Jun 2017, at 19:22, Daniel D. Daugherty <daniel.daughe...@oracle.com>
> wrote:
>
>> On 6/5/17 10:59 AM, Erik Osterlund wrote:
>> Hi Dan,
>>
>>>> On 5 Jun 2017, at 18:31, Daniel D. Daugherty <daniel.daughe...@oracle.com>
>>>> wrote:
>>>>
>>>> On 6/5/17 10:19 AM, Erik Österlund wrote:
>>>> Hi David,
>>>>
>>>>> On 2017-06-05 14:45, David Holmes wrote:
>>>>> Hi Erik,
>>>>>
>>>>>> On 5/06/2017 8:38 PM, Erik Österlund wrote:
>>>>>> Hi David,
>>>>>>
>>>>>>> On 2017-06-02 03:30, David Holmes wrote:
>>>>>>> Hi Erik,
>>>>>>>
>>>>>>>> On 2/06/2017 12:50 AM, Erik Österlund wrote:
>>>>>>>> Hi David,
>>>>>>>>
>>>>>>>>> On 2017-06-01 14:33, David Holmes wrote:
>>>>>>>>> Hi Erik,
>>>>>>>>>
>>>>>>>>> Just to be clear it is not the use of <limits> that I am concerned
>>>>>>>>> about, it is the -library=stlport4. It is the use of that flag that I
>>>>>>>>> would want to check in terms of having no affect on any existing code
>>>>>>>>> generation.
>>>>>>>> Thank you for the clarification. The use of -library=stlport4 should
>>>>>>>> not have anything to do with code generation. It only says where to
>>>>>>>> look for the standard library headers such as <limits> that are used
>>>>>>>> in the compilation units.
>>>>>>> The potential problem is that the stlport4 include path eg:
>>>>>>>
>>>>>>> ./SS12u4/lib/compilers/include/CC/stlport4/
>>>>>>>
>>>>>>> doesn't only contain the C++ headers (new, limits, string etc) but also
>>>>>>> a whole bunch of regular 'standard' .h headers that are _different_ to
>>>>>>> those found outside the stlport4 directory ie the ones we would
>>>>>>> currently include. I don't know if the differences are significant, nor
>>>>>>> whether those others may be found ahead of the stlport4 version. But
>>>>>>> that is my concern about the effects on the code.
>>>>>> While I do not think exchanging these headers will have any behavioral
>>>>>> impact, I agree that we can not prove so as they are indeed different
>>>>>> header files. That is a good point.
>>>>>>
>>>>>> However, I think that makes the stlport4 case stronger rather than
>>>>>> weaker. We already use stlport4 for our gtest testing (because it is
>>>>>> required and does not build without it). And if those headers would
>>>>>> indeed have slightly different behaviour as you imply, it further
>>>>>> motivates using the same standard library when compiling the product as
>>>>>> the testing code. If they were to behave slightly differently, it might
>>>>>> be that our gtest tests does not catch hidden bugs that only manifest
>>>>>> when building with a different set of headers used for the product
>>>>>> build. I therefore find it exceedingly dangerous to stay on two standard
>>>>>> libraries (depending on if test code or product code is compiled)
>>>>>> compared to consistently using the same standard library across all
>>>>>> compilations. So for me, the larger the risk is of them behaving
>>>>>> differently is, the bigger the motivation is to use stlport4
>>>>>> consistently.
>>>>> Regardless of what gtest does if you want to switch the standard
>>>>> libraries used by the product then IMHO that should go through a vetting
>>>>> process no weaker than that for changing the toolchain, as you
>>>>> effectively are doing that.
>>>> I talked to Erik Joelsson about how to compare two builds. He introduced
>>>> me to our compare.sh script that is used to compare two builds.
>>>> I built a baseline without these changes and a new build with these
>>>> changes applied, both on a Solaris SPARC T7 machine. Then I compared them
>>>> with ./compare.sh -2dirs {$BUILD1}/hotspot/variant-server/libjvm/objs
>>>> {$BUILD2}/hotspot/variant-server/libjvm/objs -libs --strip
>>>>
>>>> This compares the object files produced when compiling hotspot in build 1
>>>> and build 2 after stripping symbols.
>>>>
>>>> First it reported:
>>>> Libraries...
>>>> Size : Symbols : Deps : Disass :
>>>> :* diff *: : : ./dtrace.o
>>>> :* diff *: :* 38918*: ./jni.o
>>>> :* diff *: :* 23226*: ./unsafe.o
>>>>
>>>> It seems like all symbols were not stripped here on these mentioned files
>>>> and constituted all differences in the disassembly. So I made a simple sed
>>>> filter to filter out symbol names in the disassembly with the regexp <.*>.
>>>>
>>>> The result was:
>>>> Libraries...
>>>> Size : Symbols : Deps : Disass :
>>>> :* diff *: : : ./dtrace.o
>>>> :* diff *: : : ./jni.o
>>>> :* diff *: : : ./unsafe.o
>>>>
>>>> This shows that not a single instruction was emitted differently between
>>>> the two builds.
>>>>
>>>> I also did the filtering manually on jni.o and unsafe.o in emacs to make
>>>> sure I did not mess up.
>>>>
>>>> Are we happy with this, or do you still have doubts that this might result
>>>> in different code or behavior?
>>> Just to be clear: The current experiment changes both the header and
>>> the standard library right? If so, then the compare.sh run works for
>>> validating that using the new header file will not result in a change
>>> in behavior. However, that comparison doesn't do anything for testing
>>> a switch in the standard libraries right?
>> The -xnolib guards are still there in the LDFLAGS. That is, the linker will
>> not allow anything to link against either standard library. I have manually
>> confirmed this by doing the sanity check of comparing the NEEDED entries in
>> the dynamic section of the libjvm.so elf file using elfdump. It has no
>> references to neither libstlport4 nor libCstd with or without my changes.
>>
>> Summary: the changes do not add any linktime dependencies to either standard
>> library, and we are still guarded in the sense that if such dependencies
>> were to accidentally be introduced, it would not build. The only difference
>> then would be slightly different code generation of object files. And their
>> disassemblies have been confirmed not to differ by even a single instruction
>> generated differently.
>
> So your current changes use the stlport4 include path for both product
> build and 'gtest' build. You've verified the following:
>
> - The product binaries do not change even one instruction with the
> new include path.
> - The options to keep us from linking to anything in stlport4 are
> still in place.
> - You've manually verified that there are no linkage dependencies
> in the resulting binaries.
>
> If I have all that right, then I think you've covered your bases.
>
> Dan
>
>
>
>>
>> Thanks,
>> /Erik
>>
>>> Dan
>>>
>>>
>>>> Thanks,
>>>> /Erik
>>>>
>>>>> Cheers,
>>>>> David
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>> /Erik
>>>>>>
>>>>>>> Thanks,
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>
>>>>>>>> Specifically, the man pages for CC say:
>>>>>>>>
>>>>>>>> <man>
>>>>>>>> -library=lib[,lib...]
>>>>>>>>
>>>>>>>> Incorporates specified CC-provided libraries into
>>>>>>>> compilation and
>>>>>>>> linking.
>>>>>>>>
>>>>>>>> When the -library option is used to specify a CC-provided
>>>>>>>> library,
>>>>>>>> the proper -I paths are set during compilation and the
>>>>>>>> proper -L,
>>>>>>>> -Y, -P, and -R paths and -l options are set during linking.
>>>>>>>> </man>
>>>>>>>>
>>>>>>>> As we are setting this during compilation and not during linking, this
>>>>>>>> corresponds to setting the right -I paths to find our C++ standard
>>>>>>>> library headers.
>>>>>>>>
>>>>>>>> My studio friends mentioned I could double-check that we did indeed
>>>>>>>> not add a dependency to any C++ standard library by running elfdump on
>>>>>>>> the generated libjvm.so file and check if the NEEDED entries in the
>>>>>>>> dynamic section look right. I did and here are the results:
>>>>>>>>
>>>>>>>> [0] NEEDED 0x2918ee libsocket.so.1
>>>>>>>> [1] NEEDED 0x2918fd libsched.so.1
>>>>>>>> [2] NEEDED 0x29190b libdl.so.1
>>>>>>>> [3] NEEDED 0x291916 libm.so.1
>>>>>>>> [4] NEEDED 0x291920 libCrun.so.1
>>>>>>>> [5] NEEDED 0x29192d libthread.so.1
>>>>>>>> [6] NEEDED 0x29193c libdoor.so.1
>>>>>>>> [7] NEEDED 0x291949 libc.so.1
>>>>>>>> [8] NEEDED 0x291953 libdemangle.so.1
>>>>>>>> [9] NEEDED 0x291964 libnsl.so.1
>>>>>>>> [10] NEEDED 0x291970 libkstat.so.1
>>>>>>>> [11] NEEDED 0x29197e librt.so.1
>>>>>>>>
>>>>>>>> This list does not include any C++ standard libraries, as expected
>>>>>>>> (libCrun is always in there even with -library=%none, and as expected
>>>>>>>> no libstlport4.so or libCstd.so files are in there). The NEEDED
>>>>>>>> entries in the dynamic section look identical with and without my
>>>>>>>> patch.
>>>>>>>>
>>>>>>>>> I'm finding the actual build situation very confusing. It seems to me
>>>>>>>>> in looking at the hotspot build files and the top-level build files
>>>>>>>>> that -xnolib is used for C++ compilation & linking whereas
>>>>>>>>> -library=%none is used for C compilation & linking. But the change is
>>>>>>>>> being applied to $2JVM_CFLAGS which one would think is for C
>>>>>>>>> compilation but we don't have $2JVM_CXXFLAGS, so it seems to be used
>>>>>>>>> for both!
>>>>>>>> I have also been confused by this when I tried adding CXX flags
>>>>>>>> through configure that seemed to not be used. But that's a different
>>>>>>>> can of worms I suppose.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> /Erik
>>>>>>>>
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>> On 1/06/2017 7:36 PM, Erik Österlund wrote:
>>>>>>>>>> Hi David,
>>>>>>>>>>
>>>>>>>>>>> On 2017-06-01 08:09, David Holmes wrote:
>>>>>>>>>>> Hi Kim,
>>>>>>>>>>>
>>>>>>>>>>> On 1/06/2017 3:51 PM, Kim Barrett wrote:
>>>>>>>>>>>>> On May 31, 2017, at 9:22 PM, David Holmes
>>>>>>>>>>>>> <david.hol...@oracle.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Erik,
>>>>>>>>>>>>>
>>>>>>>>>>>>> A small change with big questions :)
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 31/05/2017 11:45 PM, Erik Österlund wrote:
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>> It would be desirable to be able to use harmless C++ standard
>>>>>>>>>>>>>> library headers like <limits> in the code as long as it does not
>>>>>>>>>>>>>> add any link-time dependencies to the standard library.
>>>>>>>>>>>>> What does a 'harmless' C++ standard library header look like?
>>>>>>>>>>>> Header-only (doesn't require linking), doesn't run afoul of our
>>>>>>>>>>>> [vm]assert macro, and provides functionality we presently lack (or
>>>>>>>>>>>> only handle poorly) and would not be easy to reproduce.
>>>>>>>>>>> And how does one establish those properties exist for a given
>>>>>>>>>>> header file? Just use it and if no link errors then all is good?
>>>>>>>>>> Objects from headers that are not ODR-used such as constant folded
>>>>>>>>>> expressions are not imposing link-time dependencies to C++
>>>>>>>>>> libraries. The -xnolib that we already have in the LDFLAGS will
>>>>>>>>>> catch any accidental ODR-uses of C++ objects, and the JVM will not
>>>>>>>>>> build if that happens.
>>>>>>>>>>
>>>>>>>>>> As for external headers being included and not playing nicely with
>>>>>>>>>> macros, this has to be evaluated on a case by case basis. Note that
>>>>>>>>>> this is a problem that occurs when using system headers (that we are
>>>>>>>>>> already using), as it is for using C++ standard library headers. We
>>>>>>>>>> even run into that in our own JVM when e.g. the min/max macros
>>>>>>>>>> occasionally slaps us gently in the face from time to time.
>>>>>>>>>>
>>>>>>>>>>>> The instigator for this is Erik and I are working on a project that
>>>>>>>>>>>> needs information that is present in std::numeric_limits<>
>>>>>>>>>>>> (provided
>>>>>>>>>>>> by the <limits> header). Reproducing that functionality ourselves
>>>>>>>>>>>> would require platform-specific code (with all the complexity that
>>>>>>>>>>>> can
>>>>>>>>>>>> imply). We'd really rather not re-discover and maintain
>>>>>>>>>>>> information
>>>>>>>>>>>> that is trivially accessible in every standard library.
>>>>>>>>>>> Understood. I have no issue with using <limits> but am concerned by
>>>>>>>>>>> the state of stlport4. Can you use <limits> without changing
>>>>>>>>>>> -library=%none?
>>>>>>>>>> No, that is precisely why we are here.
>>>>>>>>>>
>>>>>>>>>>>>>> This is possible on all supported platforms except the ones
>>>>>>>>>>>>>> using the solaris studio compiler where we enforce
>>>>>>>>>>>>>> -library=%none in both CFLAGS and LDFLAGS.
>>>>>>>>>>>>>> I propose to remove the restriction from CFLAGS but keep it on
>>>>>>>>>>>>>> LDFLAGS.
>>>>>>>>>>>>>> I have consulted with the studio folks, and they think this is
>>>>>>>>>>>>>> absolutely fine and thought that the choice of -library=stlport4
>>>>>>>>>>>>>> should be fine for our CFLAGS and is indeed what is already used
>>>>>>>>>>>>>> in the gtest launcher.
>>>>>>>>>>>>> So what exactly does this mean? IIUC this allows you to use
>>>>>>>>>>>>> headers for, and compile against "STLport’s Standard Library
>>>>>>>>>>>>> implementation version 4.5.3 instead of the default libCstd". But
>>>>>>>>>>>>> how do you then not need to link against libstlport.so ??
>>>>>>>>>>>>>
>>>>>>>>>>>>> https://docs.oracle.com/cd/E19205-01/819-5267/bkakg/index.html
>>>>>>>>>>>>>
>>>>>>>>>>>>> "STLport is binary incompatible with the default libCstd. If you
>>>>>>>>>>>>> use the STLport implementation of the standard library, then you
>>>>>>>>>>>>> must compile and link all files, including third-party libraries,
>>>>>>>>>>>>> with the option -library=stlport4”
>>>>>>>>>>>> It means we can only use header-only parts of the standard library.
>>>>>>>>>>>> This was confirmed / suggested by the Studio folks Erik consulted,
>>>>>>>>>>>> providing such limited access while continuing to constrain our
>>>>>>>>>>>> dependency on the library. Figuring out what can be used will
>>>>>>>>>>>> need to
>>>>>>>>>>>> be determined on a case-by-case basis. Maybe we could just link
>>>>>>>>>>>> with
>>>>>>>>>>>> a standard library on Solaris too. So far as I can tell, Solaris
>>>>>>>>>>>> is
>>>>>>>>>>>> the only platform where we don't do that. But Erik is trying to be
>>>>>>>>>>>> conservative.
>>>>>>>>>>> Okay, but the docs don't seem to acknowledge the ability to use,
>>>>>>>>>>> but not link to, stlport4.
>>>>>>>>>> Not ODR-used objects do not require linkage.
>>>>>>>>>> (http://en.cppreference.com/w/cpp/language/definition)
>>>>>>>>>> I have confirmed directly with the studio folks to be certain that
>>>>>>>>>> accidental linkage would fail by keeping our existing guards in the
>>>>>>>>>> LDFLAGS rather than the CFLAGS.
>>>>>>>>>> This is also reasonably well documented already
>>>>>>>>>> (https://docs.oracle.com/cd/E19205-01/819-5267/bkbeq/index.html).
>>>>>>>>>>
>>>>>>>>>>>>> There are lots of other comments in that document regarding
>>>>>>>>>>>>> STLport that makes me think that using it may be introducing a
>>>>>>>>>>>>> fragile dependency into the OpenJDK code!
>>>>>>>>>>>>>
>>>>>>>>>>>>> "STLport is an open source product and does not guarantee
>>>>>>>>>>>>> compatibility across different releases. In other words,
>>>>>>>>>>>>> compiling with a future version of STLport may break applications
>>>>>>>>>>>>> compiled with STLport 4.5.3. It also might not be possible to
>>>>>>>>>>>>> link binaries compiled using STLport 4.5.3 with binaries compiled
>>>>>>>>>>>>> using a future version of STLport."
>>>>>>>>>>>>>
>>>>>>>>>>>>> "Future releases of the compiler might not include STLport4. They
>>>>>>>>>>>>> might include only a later version of STLport. The compiler
>>>>>>>>>>>>> option -library=stlport4 might not be available in future
>>>>>>>>>>>>> releases, but could be replaced by an option referring to a later
>>>>>>>>>>>>> STLport version."
>>>>>>>>>>>>>
>>>>>>>>>>>>> None of that sounds very good to me.
>>>>>>>>>>>> I don't see how this is any different from any other part of the
>>>>>>>>>>>> process for using a different version of Solaris Studio.
>>>>>>>>>>> Well we'd discover the problem when testing the compiler change,
>>>>>>>>>>> but my point was more to the fact that they don't seem very
>>>>>>>>>>> committed to this library - very much a "use at own risk"
>>>>>>>>>>> disclaimer.
>>>>>>>>>> If we eventually need to use something more modern for features that
>>>>>>>>>> have not been around for a decade, like C++11 features, then we can
>>>>>>>>>> change standard library when that day comes.
>>>>>>>>>>
>>>>>>>>>>>> stlport4 is one of the three standard libraries that are presently
>>>>>>>>>>>> included with Solaris Studio (libCstd, stlport4, gcc). Erik asked
>>>>>>>>>>>> the
>>>>>>>>>>>> Studio folks which to use (for the purposes of our present
>>>>>>>>>>>> project, we
>>>>>>>>>>>> don't have any particular preference, so long as it works), and
>>>>>>>>>>>> stlport4 seemed the right choice (libCstd was, I think, described
>>>>>>>>>>>> as
>>>>>>>>>>>> "ancient"). Perhaps more importantly, we already use stlport4,
>>>>>>>>>>>> including linking against it, for gtest builds. Mixing two
>>>>>>>>>>>> different
>>>>>>>>>>>> standard libraries seems like a bad idea...
>>>>>>>>>>> So we have the choice of "ancient", "unsupported" or gcc :)
>>>>>>>>>>>
>>>>>>>>>>> My confidence in this has not increased :)
>>>>>>>>>> I trust that e.g. std::numeric_limits<T>::is_signed in the standard
>>>>>>>>>> libraries has more mileage than whatever simplified rewrite of that
>>>>>>>>>> we try to replicate in the JVM. So it is not obvious to me that we
>>>>>>>>>> should have less confidence in the same functionality from a
>>>>>>>>>> standard library shipped together with the compiler we are using and
>>>>>>>>>> that has already been used and tested in a variety of C++
>>>>>>>>>> applications for over a decade compared to the alternative of
>>>>>>>>>> reinventing it ourselves.
>>>>>>>>>>
>>>>>>>>>>> What we do in gtest doesn't necessarily make things okay to do in
>>>>>>>>>>> the product.
>>>>>>>>>>>
>>>>>>>>>>> If this were part of a compiler upgrade process we'd be comparing
>>>>>>>>>>> binaries with old flag and new to ensure there are no unexpected
>>>>>>>>>>> consequences.
>>>>>>>>>> I would not compare including <limits> to a compiler upgrade process
>>>>>>>>>> as we are not changing the compiler and hence not the way code is
>>>>>>>>>> generated, but rather compare it to including a new system header
>>>>>>>>>> that has previously not been included to use a constant folded
>>>>>>>>>> expression from that header that has been used and tested for a
>>>>>>>>>> decade. At least that is how I think of it.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> /Erik
>>>>>>>>>>
>>>>>>>>>>> Cheers,
>>>>>>>>>>> David
>>>>>>>>>>>
>>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>> David
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Webrev for jdk10-hs top level repository:
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~eosterlund/8181318/webrev.00/
>>>>>>>>>>>>>> Webrev for jdk10-hs hotspot repository:
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~eosterlund/8181318/webrev.01/
>>>>>>>>>>>>>> Testing: JPRT.
>>>>>>>>>>>>>> Will need a sponsor.
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> /Erik
>
