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

[Tim Bell]

Erik:

make/devkit/Tools.gmk

Lines 565, 566 - I see you removed dtrace from this list as that is 
dealt with up in lines 556 ... 558.  Makes sense to me.

Looks good.

Tim


> 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/
>
> /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