code review for second hotspot FDS gobjcopy work around (7165598)

[Daniel D. Daugherty]

On 5/24/12 8:28 AM, Karen Kinnear wrote:
> Dan,
>
> Code looks good.

Thanks!


> Thank you for the work-around.
>
> So is this HDR_FLAGS workaround also needed in all the other jdk repositories?

SHF_ALLOC flag?

We know that gobjcopy crashes when running into empty ELF sections
where the SHF_ALLOC flag is set. We have only seen the loader (ld)
generate empty ELF sections with the SHF_ALLOC flag set when compiling
for Solaris X64. However, it seems safer to me to always apply the
work around when building on Solaris just to be sure.

The fix_empty_sec_hdr_flags prints a diagnostic message when it
modifies an ELF object so we can check build logs periodically to
verify which Solaris builds are affected.


> And I assume the windows/makefiles/defs.make change is because you determined
> that the failing tests actually pre-dated the zipped debuginfo.

Correct. ZIP'ing the .pdb and .map files was not the cause of library
loading failures.


> Did you find the root cause for that
> yet, or is that an orthogonal topic?

Evgeny's team found that several Windows machines were missing the
msvcr71.dll library. That version of the MS runtime is needed by
the affected testbase(s) (just VM/NSK I think, not sure).


> And this also appears to have additional modifications for the earlier workaround add_gnu_debuglink.

Yes, after getting hit by the M&M test failures on Solaris 11, I
decided to apply the add_gnu_debuglink work around to every FDS
use of gobjcopy.


> I has assumed in reviewing 7165060 that you only used that for makefiles that built with DOF sections,
> i.e. libjvm.so and that you had chosen not to use the simpler add_gnu_debuglink in all cases, even though
> it is generally less risky. Did you change your approach here?

Yes, I've changed approaches due to the unexpected M&M failures.

I was originally balancing the risk of changing more Makefiles
to use the add_gnu_debuglink work around versus the risk that
gobjcopy would screw up some other ELF section in ways that we
couldn't easily detect.

In hindsight, I should have gone with using the add_gnu_debuglink
because the changes that it makes to an ELF object are rather easy
to verify as correct by comparing two elfdump output files.


> Your comment was changed from SUNW_DOF to
> SUNW_*. That makes sense to me.

Yes, Dean's guess is that the SUNW_cap section is what the M&M tests
were relying on and when gobjcopy munged it, the tests started to
fail on Solaris 11.

Interesting data points: the tests don't fail on Solaris 10 or on a
recent Solaris 11 Update 1 build (I used jurassic for a quick test).
So whatever gobjcopy did to the SUNW_cap section is properly handled
by newer Solaris 11 versions...



> And are you also changing that for
> the other jdk repositories?

Yes, please search for 7170449 in your InBox and you'll see that
review thread. I would love to have your review there also.

Dan


> thanks,
> Karen
>
> On May 23, 2012, at 3:59 PM, Daniel D. Daugherty wrote:
>
>> Greetings,
>>
>> This is a hotspot code review request for the second of a pair of
>> Full Debug Symbols gobjcopy work arounds on Solaris. The first
>> hotspot FDS gobjcopy work around was reviewed using bug 7165060
>> and that fixed the dtrace test failures.
>>
>> The gobjcopy utility also crashes due to empty sections with the
>> SHF_ALLOC flagset on Solaris X64 objects. This causes build
>> failures.
>>
>> The first new temporary work around tool is add_gnu_debuglink
>> and it was added by 7165060.The second new temporary work around
>> tool is:
>>
>> fix_empty_sec_hdr_flags - removes the SHF_ALLOC flag from empty
>>     sections in ELF objects.
>>
>> These temporary work arounds are only needed until the proper
>> Solaris 10 Update 6 patches are made available. The two patches
>> are independent of one another which is why there are two
>> separate temporary work arounds. However, we're putting the
>> temporary work arounds in place because the 7u6/HSX-23.2 project
>> window is closing fast.
>>
>> Here is the webrev URL for the HSX-24 version:
>>
>> http://cr.openjdk.java.net/~dcubed/fds_revamp/7165598-webrev/0/
>>
>> This fix will also be backported to 7u6/HSX-23.2 and I expect the
>> changes to virtually identical.
>>
>> Thanks, in advance, for any reviews!
>>
>> Dan
>>
>>