Re: [PATCH v2 01/15] Fix breakpoints in ifunc after inferior resolved it (@got.plt symbol creation)

[Simon Marchi <simark@simark.ca>]

On 2018-04-10 17:19, Pedro Alves wrote:
> On 04/01/2018 04:35 AM, Simon Marchi wrote:
>> On 2018-03-25 03:19 PM, Pedro Alves wrote:
> 
>>>    /* This search algorithm is from 
>>> _bfd_elf_canonicalize_dynamic_reloc.  */
>>>    for (relplt = obfd->sections; relplt != NULL; relplt = 
>>> relplt->next)
>>> -    if (elf_section_data (relplt)->this_hdr.sh_info == plt_elf_idx
>>> -	&& (elf_section_data (relplt)->this_hdr.sh_type == SHT_REL
>>> -	    || elf_section_data (relplt)->this_hdr.sh_type == SHT_RELA))
>>> -      break;
>>> +    {
>>> +      const auto &this_hdr = elf_section_data (relplt)->this_hdr;
>>> +
>>> +      if (this_hdr.sh_type == SHT_REL || this_hdr.sh_type == 
>>> SHT_RELA)
>>> +	{
>>> +	  asection *target_section = NULL;
>>> +
>>> +	  if (this_hdr.sh_info == plt_elf_idx)
>>> +	    target_section = plt;
>>> +	  else if (this_hdr.sh_info == got_plt_elf_idx)
>>> +	    target_section = got_plt;
>>> +
>>> +	  if (target_section != NULL)
>>> +	    break;
>> 
>> Is it really useful to have/set target_section?  Couldn't we just 
>> break out of the
>> loop like this?
>> 
>>   if (this_hdr.sh_info == plt_elf_idx
>>       || this_hdr.sh_info == got_plt_elf_idx)
>>     break;
>> 
> 
> Hmm, the original intention was to use target_section in the other
> loop, but that didn't work, so I reverted it, but somehow not that
> part.  :-P
> 
>>> 
>>> @@ -573,6 +586,18 @@ elf_rel_plt_read (minimal_symbol_reader &reader,
>>> 
>>>    std::string string_buffer;
>>> 
>>> +  /* Does ADDRESS reside in SECTION of OBFD?  */
>>> +  auto within_section = [obfd] (asection *section, CORE_ADDR 
>>> address)
>>> +    {
>>> +      if (section == NULL)
>>> +	return false;
>>> +
>>> +      /* Does the pointer reside in the .got.plt section?  */
>> 
>> That comment should change, since it's not stricly .got.plt.
>> 
> 
> I've removed it, since the intro comment already says it all.
> 
>> Or maybe you intended to use target_section here at some point?  Is 
>> there a
>> relationship between the section that matched in the for loop above 
>> and the
>> section that will contain the address?  In other words, could we save 
>> the
>> target_section from above and do
> 
> Yeah, in an earlier version I tried doing that, but then testing on the
> different systems found out that there's no relation between the
> two sections.
> 
> Here's the updated patch.  WDYT?

LGTM (though I trust the passing tests more than I trust myself).

Simon