RFR: JDK-8198243: Add build time check for global operator new/delete in object files

[Kim Barrett]

> On Mar 5, 2018, at 11:41 AM, Erik Joelsson <erik.joelsson at oracle.com> wrote:
> 
> Hello,
> 
> On 2018-03-04 15:22, David Holmes wrote:
>> Hi Erik,
>> 
>> On 3/03/2018 7:18 AM, Erik Joelsson wrote:
>>> Hello,
>>> 
>>> Here is a new version of this patch, reworked in several ways. It now supports gcc, clang and solstudio. It uses nm instead of objdump, which is more readily available in all our current build environments. The check now uses mangled symbol names for all toolchain types which makes things considerably faster. I also added a check for only finding symbols classified as undefined. Otherwise we get false positives in operator_new.o in debug builds.
>> 
>> Sounds great! Thanks for doing the additional work on this.
>> 
>>> I left the objdump addition to the devkit in there because it's a good thing to have anyway for compare.sh.
>>> 
>>> Webrev: http://cr.openjdk.java.net/~erikj/8198243/webrev.02/
>> 
>> In toolkit.m4:
>> 
>> +   case $TOOLCHAIN_TYPE in
>> +     gcc|clang|solstudio)
>>         BASIC_CHECK_TOOLS(OBJDUMP, [gobjdump objdump])
>>         if test "x$OBJDUMP" != x; then
>>           # Only used for compare.sh; we can live without it. BASIC_FIXUP_EXECUTABLE
>>           # bails if argument is missing.
>>           BASIC_FIXUP_EXECUTABLE(OBJDUMP)
>>         fi
>> +       BASIC_FIXUP_EXECUTABLE(OBJDUMP)
>> 
>> Isn't the if-clause redundant now that you unconditionally call BASIC_FIXUP_EXECUTABLE(OBJDUMP)?
>> 
> Yes, I seem to have accidentally left the second line there. My intention was to restore the conditional now that objdump is no longer used for this new check.
> 
> Here is a new webrev with the second fixup call removed:
> 
> http://cr.openjdk.java.net/~erikj/8198243/webrev.03/

Old code in toolkit.m4 did the OBJDUMP stuff unconditionally.  New code makes it conditional on TOOLCHAIN_TYPE.
That doesn’t seem right.  compare.sh seems to want OBJDUMP for aix too, and that’s not in the toolchain list here.

> 
> /Erik
>> Thanks,
>> David
>> 
>>> I have run this patch against current jdk/jdk and jdk/hs in Mach5. In jdk/jdk the build fails as expected on Solaris, Linux and Mac and in jdk/hs, where the fixes we are detecting are present, all builds succeeds.
>>> 
>>> /Erik
>>> 
>>> 
>>> On 2018-02-23 05:27, Magnus Ihse Bursie wrote:
>>>> On 2018-02-22 20:41, Erik Joelsson wrote:
>>>>> 
>>>>> 
>>>>> On 2018-02-21 21:06, David Holmes wrote:
>>>>>> On 22/02/2018 4:07 AM, Erik Joelsson wrote:
>>>>>>> Hello,
>>>>>>> 
>>>>>>> 
>>>>>>> On 2018-02-20 21:33, David Holmes wrote:
>>>>>>>> 
>>>>>>>> a) how much time it adds to the build?
>>>>>>>> 
>>>>>>> I have not done extensive testing, but on my Linux workstation with 32 hw threads, building just hotspot release build from a clean workspace increased maybe 1 or 2 seconds (at around 90s total), but the variance was around the same amount as that.
>>>>>>>> b) why this doesn't work for Solaris Studio?
>>>>>>>> 
>>>>>>> I didn't put a lot of effort into trying to figure it out. The check used was provided by Kim Barrett, for Linux only. I figured it would be simple enough to get it to work on mac and succeeded there. It should certainly be possible to implement a similar check on Solaris, but is it worth the time to do it? Both development time and increased build time on one of the slower build platforms?
>>>>>> 
>>>>>> Depends how concerned we are with detecting this problem in OS specific source code?
>>>>>> 
>>>>> I investigated this some more. I was able to do it successfully, but the build time cost is way too large here. The culprit is c++filt on Solaris which is incredibly costly, and the gnu version does not demangle Solaris Studio symbols. I don't think we should do this on Solaris.
>>>> I agree, it's not worth it.
>>>> 
>>>> Not all programmer's mistakes are reasonable to catch in technical traps. It we *should* spend time on getting automatic tool for keeping code quality up (and, yes, I really do think we should), there's most likely to be much better areas to spend that effort in, in making a lot of prone-to-break scripts for catching a single kind of problem.
>>>> 
>>>> /Magnus
>>>> 
>>>>> 
>>>>> We could grep for the mangled strings for the operators instead, which is super fast. Problem is just figuring out all the possible combinations.
>>>>> 
>>>>> /Erik
>>>>>> To be honest I'm not sure this pulls its own weight regardless.
>>>>>> 
>>>>>> David
>>>>>> 
>>>>>>> /Erik
>>>>>>>> Thanks,
>>>>>>>> David
>>>>>>>> 
>>>>>>>> On 21/02/2018 4:05 AM, Erik Joelsson wrote:
>>>>>>>>> Hello,
>>>>>>>>> 
>>>>>>>>> This patch adds a build time check for uses of global operators new and delete in hotspot C++ code. The check is only run with toolchains GCC and Clang (Linux and Macos builds). I have also modified the Oracle devkit on Linux to add the necessary symlink so that objdump will get picked up by configure.
>>>>>>>>> 
>>>>>>>>> This change is depending on several fixes removing such uses that are currently in jdk/hs so this change will need to be pushed there as well.
>>>>>>>>> 
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8198243
>>>>>>>>> 
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~erikj/8198243/webrev.01/
>>>>>>>>> 
>>>>>>>>> /Erik