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

Craig Blackmore <[email protected]> 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.

> diff --git a/gdb/testsuite/gdb.base/bp-non-alloc.exp b/gdb/testsuite/gdb.base/bp-non-alloc.exp
> new file mode 100644
> index 00000000000..7758a591fa7
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/bp-non-alloc.exp
> @@ -0,0 +1,36 @@
> +# Copyright (C) 2026 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# For an ELF that has no section called ".text" and the first section is
> +# non-alloc, test that a breakpoint can be set on a function.  This previously
> +# caused GDB to crash due to a missing null pointer check.
> +
> +require is_elf_target
> +
> +global srcdir
> +global subdir
> +
> +standard_testfile
> +
> +set linker_script $srcdir/$subdir/$testfile.ld
> +
> +set options "debug ldscript=-Wl,-T${linker_script}"
> +if {[build_executable "failed to prepare" $testfile $srcfile $options]} {
> +    return -1

The '-1' here can be dropped.

> +}
> +
> +clean_restart $testfile
> +
> +gdb_test "break main" "Breakpoint .* at .*"
> diff --git a/gdb/testsuite/gdb.base/bp-non-alloc.ld b/gdb/testsuite/gdb.base/bp-non-alloc.ld
> new file mode 100644
> index 00000000000..6a8ad57af18
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/bp-non-alloc.ld
> @@ -0,0 +1,35 @@
> +/* Copyright (C) 2026 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +   This linker script is used to produce an executable that starts with a
> +   non-allocatable section and does not contain a `.text` section.  */
> +
> +MEMORY
> +{
> +  DATA (rw) : ORIGIN = 0x8000000, LENGTH = 0x10000
> +  TEXT (rx) : ORIGIN = LENGTH (DATA), LENGTH = 0x10000

This seems a little strange.  The ORIGIN of TET will be set to 0x10000
will it not?  Which places TEXT before DATA.  Now there's nothing wrong
with that at all, but the ordering here seems weird.  And also having
the ORIGIN of TEXT depend on a LENGTH when it's going to be placed
earlier in memory seems unnecessary, you'd be better just saying
'ORIGIN=0x10000' if that's what you mean.

But maybe you actually meant something different?

Thanks,
Andrew


> +}
> +
> +SECTIONS
> +{
> +  .my_non_alloc_sec (INFO) : { . = . + 0x10; }
> +  .text.all : { *(.text) } > TEXT
> +  .data : { *(.data) } > DATA
> +  _edata = .;
> +  .bss : { *(.bss) } > DATA
> +  _end = .;
> +}
> -- 
> 2.43.0