RFR: JDK-8142907 Integration of minor fixes from the build-infra project

[Gary Adams]

On 12/14/15 07:34, David Holmes wrote:
> On 14/12/2015 6:30 PM, Magnus Ihse Bursie wrote:
>> On 2015-12-14 09:12, David Holmes wrote:
>>> Hi Magnus,
>>>
>>> On 12/12/2015 8:24 AM, Magnus Ihse Bursie wrote:
>>>> On 2015-12-11 22:43, Magnus Ihse Bursie wrote:
>>>>> Hi David,
>>>>>
>>>>> Resurrecting the second part of the build-infra integrations. :)
>>>>> Fortunately, no major code changes in the meantime has affected this
>>>>> patch. (I did need to update the new
>>>>> hotspot/make/lib/Lib-jdk.hotspot.agent.gmk though.)
>>>>
>>>> It seems I forgot to include the link to the new webrev:
>>>>
>>>> http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.05 
>>>>
>>>>
>>>
>>> hotspot.m4:
>>>
>>>  54 #    server: normal interpreter and a tiered C1/C2 compiler
>>>   55 #    client: normal interpreter and C1 (no C2 compiler) (only
>>> 32-bit platforms)
>>>
>>> Neither of these statements are necessarily 100% true - not all
>>> platforms have tiered enable; some 64-bit platforms do (or will)
>>> support client.
>>
>> This code was directly moved from jdk-options.m4. But I can update the
>> text to better fit reality. I suggest updating the comment about 
>> server to:
>> # server: normal interpreter, and a C2 or tiered C1/C2 compiler
>> depending on platform
>> Ok?
>
> Ok.
>
>> However, a few lines below we explicitely block enabling of client on
>> 64-bit platforms, so I believe this statement is correct, at least for
>> the code as it looks now. It might be the case that the configure check
>> is too strict, and if so we should modify it, but as long as the check
>> remains I believe the comment should remain. Ok?
>
> OK. As long as whomever removes the check knows to remove the comment. 
> Both the arm32 port and the mobile project will be relaxing this afaik.
For the mobile port we have relaxed the constraint and allow a 64 bit 
client and
minimal vm to be built. I'll make sure the comment is updated in our repos.
I don't think there was a fundamental reason those variations were not 
support,
just a question of which platforms were being built and tested on a 
regular basis.

>
> Thanks,
> David
>
>>> Otherwise I didn't see anything else that I obviously needed to
>>> comment on. The proof of this is in the building. So as long as this
>>> gets through JPRT with -testset hotspot we should be okay. :)
>> Great. Thanks for the review!
>>
>> /Magnus
>>>
>>> Thanks,
>>> David
>>>
>>>>
>>>> /Magnus
>>>>
>>>>>
>>>>>
>>>>> On 2015-11-16 05:33, David Holmes wrote:
>>>>>> Hi Magnus,
>>>>>>
>>>>>> I had a flick through most of the files. Overall seems okay but I
>>>>>> have a few queries below.
>>>>>
>>>>> Replies inline.
>>>>>
>>>>>>
>>>>>> On 13/11/2015 12:43 PM, Magnus Ihse Bursie wrote:
>>>>>>> The build-infra project has collected a number of minor fixes and
>>>>>>> changes during the new hotspot build development. It's a mix of
>>>>>>> code
>>>>>>> cleanup and new capabilities.
>>>>>>>
>>>>>>> Not all of these new features are immediately beneficial to the 
>>>>>>> JDK,
>>>>>>> but
>>>>>>> they will be needed for the upcoming new Hotspot build, and it
>>>>>>> will not
>>>>>>> hurt to have them in mainline. (In fact, it will tremendously help
>>>>>>> merging between mainline and build-infra.)
>>>>>>>
>>>>>>> The fix addresses these issues:
>>>>>>>
>>>>>>> In general:
>>>>>>> * Break out hotspot configuration into hotspot.m4
>>>>>>> * Long link lines uses @-files
>>>>>>> * Consistently use -Wl instead of -Xlinker
>>>>>>> * Improve clang on linux compilation
>>>>>>> * Set shared library name explicitely on solaris
>>>>>>> * Set correct shared library flag on Windows (-dll)
>>>>>>> * Consistency fixes for build toolchain
>>>>>>> * Bring compare script up to date
>>>>>>> * General code/whitespace cleanup
>>>>>>> * Additional functionality in MakeBase
>>>>>>>
>>>>>>> In NativeCompilation.gmk:
>>>>>>> * More efficient vardeps for per-file CFLAGS
>>>>>>> * Fewer shell executions (means better performance on Windows)
>>>>>>> * EXCLUDE_PATTERN and EXTRA_OBJECT_FILES
>>>>>>> * Debug symbols on macosx (disabled for existing code to keep 
>>>>>>> current
>>>>>>> behavior)
>>>>>>>
>>>>>>> Enabling debug info on macosx on existing jdk should be treated 
>>>>>>> in a
>>>>>>> follow-up bug.
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8142907
>>>>>>> WebRev:
>>>>>>> http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.01 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> (It turned out that WebRev could not at the same time include files
>>>>>>> from
>>>>>>> multiple repos and track the history of a "hg cp":ied file. So I
>>>>>>> created
>>>>>>> an alternative revision here:
>>>>>>> http://cr.openjdk.java.net/~ihse/JDK-8142907-build-infra-integration-closed/webrev.02/ 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> It does not include the jdk files, but hotspot.m4 might be 
>>>>>>> easier to
>>>>>>> understand)
>>>>>>
>>>>>> flags.m4:
>>>>>>
>>>>>>   60   AC_SUBST(LEGACY_EXTRA_CFLAGS)
>>>>>>   61   AC_SUBST(LEGACY_EXTRA_CXXFLAGS)
>>>>>>   62   AC_SUBST(LEGACY_EXTRA_LDFLAGS)
>>>>>>   63
>>>>>>   64   AC_SUBST(EXTRA_CFLAGS)
>>>>>>   65   AC_SUBST(EXTRA_CXXFLAGS)
>>>>>>   66   AC_SUBST(EXTRA_LDFLAGS)
>>>>>>
>>>>>> IIRC we added the legacy flags purely to pass the cross-compilation
>>>>>> args through to hotspot. Not sure why we need both legacy and
>>>>>> non-legancy variants now ??
>>>>>
>>>>> This is part of an ongoing effort to split CFLAGS_JDK into more
>>>>> reasonable parts. The current model is that we throw all possible
>>>>> flags together in CFLAGS_JDK (and then further on to CFLAGS_JDKLIB et
>>>>> al). While it was questionable even before, it did work when we only
>>>>> built the JDK native libraries. Now that we need to build 
>>>>> libjvm.so as
>>>>> well, which has (for historical reason) a set of flags that are both
>>>>> partially the same and partially different, this is not so good.
>>>>>
>>>>> We would like to see a change to a situation where the different
>>>>> "parts" that we build CFLAGS_JDK from would be handled separately, 
>>>>> and
>>>>> then combined as needed for JDK libraries, and for Hotspot. So,
>>>>> currently we use EXTRA_CFLAGS just to add them to CFLAGS_JDK, and 
>>>>> then
>>>>> drop them. In the new Hotspot build, we use the EXTRA_CFLAGS to add
>>>>> them to the Hotspot flags. Of course, we could have used
>>>>> LEGACY_EXTRA_CFLAGS, but we'd like to avoid using "LEGACY" for new
>>>>> stuff.
>>>>>
>>>>> With that being said, I now realize that maybe this change will 
>>>>> not be
>>>>> needed anyway, at least not right now. Since doing a proper flag
>>>>> cleanup is a major task, we decided to copy the CFLAGS_JDK behavior
>>>>> for Hotspot flags as well in the current implementation of the new
>>>>> hotspot build, and saving the cleanup for another day (so it will not
>>>>> block the new build system). So I'll revert this part of the change
>>>>> for now.
>>>>>
>>>>>> On Windows -LD is a superset of -dll, so it isn't obvious the change
>>>>>> is correct.
>>>>> I hope Erik's reply to that was satisfactory.
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> jdk/make/lib/LibCommon.gmk
>>>>>>
>>>>>> + # Disable it here for the jdk binaries until we decide to enable
>>>>>> them.
>>>>>>
>>>>>> s/binaries/libraries/ ?
>>>>> Sure, I'll fix.
>>>>>
>>>>>> Actually both this fragment and the one in
>>>>>> jdk/make/launcher/LauncherCommon.gmk I find confusing - what is the
>>>>>> relation with hotspot here, and the role of SetupNativeCompilation?
>>>>> I hope Erik's reply to that is acceptable as well. As for the other
>>>>> changes in the jdk repo, most of them is to either standardize on 
>>>>> -Wl,
>>>>> or to fix some places were we didn't do a proper $$ (instead of $) in
>>>>> an macro that was supposed to be eval'd. (This was incorrect 
>>>>> before as
>>>>> well, but it broke the build when the LDFLAGS started having 
>>>>> commas in
>>>>> them.)
>>>>>
>>>>> /Magnus
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>
>>>>>>> /Magnus
>>>>>
>>>>
>>