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

[Erik Joelsson]

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/

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