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

[David Holmes]

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)?

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