Re: [PATCH] Add missing null pointer check in get_sal_arch

[Craig Blackmore <craig.blackmore@embecosm.com>]

[-- Attachment #1: Type: text/plain, Size: 3406 bytes --]

Hi Andrew,

On 31/07/2026 16:27, Andrew Burgess wrote:
> Andrew Burgess<aburgess@redhat.com> writes:
>
>> Craig Blackmore<craig.blackmore@embecosm.com> writes:
>>
>>> This fixes a GDB crash when trying to set a breakpoint on a function in
>>> an ELF where there is both no .text section and the first section within
>>> the ELF is not allocatable.
>> This tells us WHAT happened, but not WHY.  We understand the input as
>> you gave a description of the ELF, and you explained the end result, a
>> crash.  But it would be really useful if you could fill in the middle
>> bit.  Why does the objfile end up as NULL?
>>
>> When a fix is "add a NULL pointer check" my immediate question is: should the pointer even be NULL? Maybe 
>> there's a better fix elsewhere in GDB which prevents the pointer from 
>> ever becoming NULL. The goal of the "middle bit" that I asked for above is to convince the reviewers
>> that NULL is a valid possibility and that a NULL check should be added.
>>
>> This commit from April seems like it might be in a similar area of GDB:
>>
>>    commit cd289df068e39683576f95907b5dd06ae3e4e254
>>    Date:   Wed Apr 15 10:43:31 2026 +0100
>>
>>      gdb: don't use .text as default entry point section
>>
>> and might be worth a read.
> I looked at this a bit more and `init_objfile_sect_indices` ends with
> this code:
>
>    for (i = 0; i < objfile->section_offsets.size (); i++)
>      {
>        if (objfile->section_offsets[i] != 0)
> 	{
> 	  break;
> 	}
>      }
>    if (i == objfile->section_offsets.size ())
>      {
>        if (objfile->sect_index_text == -1)
> 	objfile->sect_index_text = 0;
>        if (objfile->sect_index_data == -1)
> 	objfile->sect_index_data = 0;
>        if (objfile->sect_index_bss == -1)
> 	objfile->sect_index_bss = 0;
>        if (objfile->sect_index_rodata == -1)
> 	objfile->sect_index_rodata = 0;
>      }
>
> With the idea being that if every section has a relocation offset of
> zero then we can just point at any section.  That's fine as far as the
> actual relocation offset is concerned, but sect_index_text is also used
> to find an objfile, and in this case, we need to point to an actual
> allocatable section.
>
> Maybe we should rewrite the 'if (objfile->sect_index_text == -1)' case
> so instead of always selecting index 0 we select the first allocatable
> and executable section?  I had a go at this, see the patch below, and
> your test case still passes.
>
> I also wondered if we should be adding an assert to catch this
> problematic case earlier on?  In
> buildsym_compunit::finish_block_internal where we do:
>
>        symbol->set_section_index (SECT_OFF_TEXT (m_objfile));
>
> this seems to be the first point where we could spot the problem maybe
> as this is where the offset to the wrong section is used for a symbol.
> Maybe here, or close to here, we could have an assert that the symbol
> has a valid objfile?  I haven't exactly figured this bit out, but could
> be something to investigate.
>
> Anyway, let me know what you think of this alternative approach.

Thanks for looking into this and sharing your analysis and alternative
fix. I much prefer your fix as it addresses the root cause and it works
on both my original test and the updated test that I just posted.

As you've written the fix, would you prefer to add my test to your patch
or for me to combine everything into a new submission?

Thanks,
Craig

[-- Attachment #2: Type: text/html, Size: 4588 bytes --]