Request for approval: Allow Java's ELF symtab reader to use separate debuginfo files

[Tom Rodriguez]

On Jan 9, 2010, at 3:03 AM, Andrew Haley wrote:

> On 01/08/2010 07:54 PM, Tom Rodriguez wrote:
> 
>> I think a better fix is to add the required gHotSpot* symbols to the
>> mapfile so that they are always exported.
> 
> I'm not sure this will work on all GNU/Linux distros, since symtabs
> are routinely removed in their entirety from DSOs.  Is there some
> logic in the symtab handling to do a look-aside into the dynamic
> symtab instead of the symtab?

The solaris libproc does this and I assumed that the linux code we wrote would also do this but it looks to me like it doesn't.  I don't think it would be hard to add support for parsing the dynsym table in build_symtab and it would allow these tools to work if the libraries are commonly stripped.  The code could either be modified to look for SHT_DYNSYM if SHT_SYMTAB doesn't exist or it could always parse both of them.  Something limited like this works ok:

diff -r f24201449cac agent/src/os/linux/symtab.c                                                                                       
--- a/agent/src/os/linux/symtab.c       Sat Jan 09 00:59:35 2010 -0800                                                                 
+++ b/agent/src/os/linux/symtab.c       Mon Jan 11 12:34:56 2010 -0800                                                                 
@@ -66,6 +66,7 @@ struct symtab* build_symtab(int fd) {
   ELF_SHDR* cursct = NULL;                                                                                                             
   ELF_PHDR* phbuf = NULL;                                                                                                             
   ELF_PHDR* phdr = NULL;                                                                                                               
+  int sym_section = SHT_DYNSYM;                                                                                                       
                                                                                                                                       
   uintptr_t baseaddr = (uintptr_t)-1;                                                                                                 
                                                                                                                                       
@@ -90,9 +91,13 @@ struct symtab* build_symtab(int fd) {
                                                                                                                                       
   for (cursct = shbuf, cnt = 0; cnt < ehdr.e_shnum; cnt++) {                                                                           
     scn_cache[cnt].c_shdr = cursct;                                                                                                   
-    if (cursct->sh_type == SHT_SYMTAB || cursct->sh_type == SHT_STRTAB) {                                                             
+    if (cursct->sh_type == SHT_SYMTAB || cursct->sh_type == SHT_STRTAB || cursct->sh_type == SHT_DYNSYM) {                             
       if ( (scn_cache[cnt].c_data = read_section_data(fd, &ehdr, cursct)) == NULL) {                                                   
          goto quit;                                                                                                                   
+      }                                                                                                                               
+      if (cursct->sh_type == SHT_SYMTAB) {                                                                                             
+        // Full symbol table available so use that                                                                                     
+        sym_section = cursct->sh_type;                                                                                                 
       }                                                                                                                               
     }                                                                                                                                 
     cursct++;                                                                                                                         
@@ -101,7 +106,7 @@ struct symtab* build_symtab(int fd) {
   for (cnt = 1; cnt < ehdr.e_shnum; cnt++) {                                                                                           
     ELF_SHDR *shdr = scn_cache[cnt].c_shdr;                                                                                           
                                                                                                                                       
-    if (shdr->sh_type == SHT_SYMTAB) {                                                                                                 
+    if (shdr->sh_type == sym_section) {                                                                                               
       ELF_SYM  *syms;                                                                                                                 
       int j, n, rslt;                                                                                                                 
       size_t size;


> 
> Also, are these the only symbols that are ever looked up by this
> symtab reader?

Those are the only ones needed by the core SA functionality.  The only thing other symbols are used for is the jstack -m mode that dumps mixed Java and native frames ala pstack and adding the external debug info reader to support this seems reasonable.

tom

> 
>> This way jmap would work even if the debug symbols for libjvm.so
>> weren't installed on the machine.  I actually thought we already did
>> this because we explicitly export a few vtbls for use by the SA but
>> apparently we don't.  I must have been thinking of windows where you
>> have to explicitly export them or they won't show up at all.
>> Anyway, adding this to the mapfiles will allow it work as is:
>> 
>>                # SA symbols
>>                gHotSpotVMIntConstantEntryArrayStride;
>>                gHotSpotVMIntConstantEntryNameOffset;
>>                gHotSpotVMIntConstantEntryValueOffset;
>>                gHotSpotVMIntConstants;
>>                gHotSpotVMLongConstantEntryArrayStride;
>>                gHotSpotVMLongConstantEntryNameOffset;
>>                gHotSpotVMLongConstantEntryValueOffset;
>>                gHotSpotVMLongConstants;
>>                gHotSpotVMStructEntryAddressOffset;
>>                gHotSpotVMStructEntryArrayStride;
>>                gHotSpotVMStructEntryFieldNameOffset;
>>                gHotSpotVMStructEntryIsStaticOffset;
>>                gHotSpotVMStructEntryOffsetOffset;
>>                gHotSpotVMStructEntryTypeNameOffset;
>>                gHotSpotVMStructEntryTypeStringOffset;
>>                gHotSpotVMStructs;
>>                gHotSpotVMTypeEntryArrayStride;
>>                gHotSpotVMTypeEntryIsIntegerTypeOffset;
>>                gHotSpotVMTypeEntryIsOopTypeOffset;
>>                gHotSpotVMTypeEntryIsUnsignedOffset;
>>                gHotSpotVMTypeEntrySizeOffset;
>>                gHotSpotVMTypeEntrySuperclassNameOffset;
>>                gHotSpotVMTypeEntryTypeNameOffset;
>>                gHotSpotVMTypes;
>> 
>> Would this be acceptable?
> 
> I'll try it and report back.
> 
> It seems a bit fragile, in that it depends on having a few magic names
> and it won't get tested in the standard build.  I considered this
> option but rejected it in favour of fixing the root cause of the
> problem, which was that the symtab reader wasn't looking in the right
> place.
> 
> Having said that, it's nice not to require the debuginfo files.
> 
> Andrew.