Hi,

On 08/09/2013 12:08 PM, Pedro Alves wrote:
> On 08/08/2013 05:41 PM, Luis Machado wrote:
>
>> Here is an updated patch that deals with solib-dsbt.c as well. The
>> additional tic6x-specific fields from the loadmap were removed and
>> dsbt_index is now fetched from the shared library file on disk instead
>> of grabbing such information from the runtime loadmap. That information
>> does not change anyway.
>
> Is there no DT_DEBUG in auvx in FDPIC/DSBT we could consult first?
>

Possibly. The main problem here is making sure whatever change we make 
works reliably without access to real hardware for testing. The code in 
question, to read data from the file, is a bit simpler.

>>
>> +/* Scan for DYNTAG in .dynamic section of ABFD. If DYNTAG is found 1
>
> Double space after period.
>

Silly mistake always there.

>> +   is returned and the corresponding PTR is set.  We only search in
>> +   the BFD, not in the target's memory.  */
>> +
>> +static int
>> +scan_dyntag_in_bfd (int dyntag, bfd *abfd, CORE_ADDR *ptr)
>> +{
>> +  int step, sect_size;
>> +  long dyn_tag;
>> +  CORE_ADDR dyn_ptr, dyn_addr;
>> +  gdb_byte *bufend, *bufstart, *buf;
>> +  Elf64_External_Dyn *x_dynp_64;
>> +  struct bfd_section *sect;
>> +  struct target_section *target_section;
>> +
>> +  if (abfd == NULL)
>> +    return 0;
>> +
>> +  if (bfd_get_flavour (abfd) != bfd_target_elf_flavour)
>> +    return 0;
>> +
>> +  /* Make sure the size is sane.  */
>> +  if (bfd_get_arch_size (abfd) == -1)
>> +    return 0;
>> +
>> +  /* Find the start address of the .dynamic section.  */
>> +  sect = bfd_get_section_by_name (abfd, ".dynamic");
>> +  if (sect == NULL)
>> +    return 0;
>> +
>> +  for (target_section = current_target_sections->sections;
>> +       target_section < current_target_sections->sections_end;
>> +       target_section++)
>> +    if (sect == target_section->the_bfd_section)
>> +      break;
>> +  if (target_section < current_target_sections->sections_end)
>> +    dyn_addr = target_section->addr;
>> +  else
>> +    {
>> +      /* ABFD may come from OBJFILE acting only as a symbol file
>> +	 without being loaded into the target
>> +	 (see add_symbol_file_command).  This case is such fallback
>> +	 to the file VMA address without the possibility of having
>> +	 the section relocated to its actual in-memory address.  */
>> +
>> +      dyn_addr = bfd_section_vma (abfd, sect);
>> +    }
>> +
>> +  /* Read in .dynamic from the BFD.  */
>> +  sect_size = bfd_section_size (abfd, sect);
>> +  buf = bufstart = alloca (sect_size);
>> +  if (!bfd_get_section_contents (abfd, sect,
>> +				 buf, 0, sect_size))
>> +    return 0;
>> +
>> +  /* Iterate over BUF and scan for DYNTAG.  If found, set PTR and
>> +     return.  */
>> +  step = sizeof (Elf64_External_Dyn);
>> +
>> +  for (bufend = buf + sect_size;
>> +       buf < bufend;
>> +       buf += step)
>> +  {
>> +    x_dynp_64 = (Elf64_External_Dyn *) buf;
>> +    dyn_tag = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_tag);
>> +    dyn_ptr = bfd_h_get_64 (abfd, (bfd_byte *) x_dynp_64->d_un.d_ptr);
>> +
>> +    if (dyn_tag == DT_NULL)
>> +      return 0;
>> +    if (dyn_tag == dyntag)
>> +      {
>> +	if (ptr)
>
> 	if (ptr != NULL)
>
>> +	  {
>> +	    /* We don't want to read this information from memory.
>> +	       If a relocation is needed, it should be done
>> +	       elsewhere.  */
>> +	    *ptr = dyn_ptr;
>> +	  }
>> +	return 1;
>> +      }
>> +  }
>> +
>> +  return 0;
>> +}
>> +
>
> Would solib-svr4.c's scan_dyntag work pristine here?
> This duplication is making me like Maciej's idea of a
> shared-between-elf-ish-solib-backends solib-elf.c file more.
>

It needs some tweaking to take into account relocations and it needs 
some help to locate the shared library's .dynamic section. From there we 
can extract the DSBT_INDEX. But, once again, the problem here is making 
sure such a change works reliably without hardware to test on.

>> +/* Given a shared library filename, load it up and find
>> +   out what is its dsbt index.  */
>> +
>> +static int
>> +fetch_solib_dsbt_index (char *filename)
>
> could be const?
>

Aye.

>> +      target_read_string (addr, &name_buf, SO_NAME_MAX_PATH_SIZE - 1,
>> +			  &errcode);
>> +
>> +      if (errcode != 0)
>> +	dsbt_index = 0;
>
> Is this an error path?  Should it still have the warning removed below?
>

Yeah. I've pulled the path warning from the inner block to the outer 
one. It also warns about the lack of a dsbt_index. The default index in 
case of error is -1 now. Only the main executable can have dsbt index 0, 
and there can be only one.

>> +      else
>>   	{
>> -	  warning (_("dsbt_current_sos: Unable to read dsbt index."
>> -		     "  Shared object chain may be incomplete."));
>> -	  break;
>> +	  /* Fetch the DSBT index of this load module.  */
>> +	  dsbt_index = fetch_solib_dsbt_index (name_buf);
>>   	}
>> -      dsbt_index = extract_unsigned_integer (indexword, sizeof indexword,
>> -					     byte_order);
>>
>
> the bit where that above was moved from:
>
>>   	  sop->lm_info->map = loadmap;
>> -	  /* Fetch the name.  */
>> -	  addr = extract_unsigned_integer (lm_buf.l_name,
>> -					   sizeof (lm_buf.l_name),
>> -					   byte_order);
>> -	  target_read_string (addr, &name_buf, SO_NAME_MAX_PATH_SIZE - 1,
>> -			      &errcode);
>>
>>   	  if (errcode != 0)
>>   	    warning (_("Can't read pathname for link map entry: %s."),
>
> had the warning, but it isn't reachable, due to
>
>    if (dsbt_index != 0)
>
> above.
>
> Otherwise this is fine with me, if fine with Yao.
>

How does this version look?

Thanks for reviewing.