> On Mar 5, 2018, at 11:41 AM, Erik Joelsson <erik.joels...@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
