Re: [PATCH] linux: Add maintenance commands to test libthread_db

[Pedro Alves <palves@redhat.com>]

On 11/22/2017 02:42 PM, Gary Benson wrote:

> 
> Built and regtested on RHEL 7.4 x86_64.
> 
> Ok to commit?

I noticed you're missing a NEWS entry.

Some more comments below.

> +static int
> +check_thread_db_callback (const td_thrhandle_t *th, void *arg)
> +{
> +  gdb_assert (tdb_testinfo != NULL);
> +  tdb_testinfo->threads_seen = true;
> +
> +#define LOG(fmt, args...)						\
> +  do									\
> +    {									\
> +      if (tdb_testinfo->log_progress)					\
> +	{								\
> +	  debug_printf (fmt, ## args);					\
> +	  gdb_flush (gdb_stdlog);					\
> +	}								\
> +    }									\
> +  while (0)
> +
> +#define __CHECK(expr, args...)						\

Symbols that start with __ are reserved for the C/C++ implementation.

(E.g., glibc would be in the right to define it in some header
for its own purposes).

You can rename it to CHECK_1 or something like that.

> +  do									\
> +    {									\
> +      if (!(expr))							\
> +	{								\
> +	  LOG (" ... FAIL!\n");						\
> +	  error (args);							\
> +	}								\
> +    }									\
> +  while (0)
> +
> +#define CHECK(expr)							\
> +  __CHECK (expr, "(%s) == false", #expr)
> +
> +#define CHECK_CALL(func, args...)					\
> +  do									\
> +    {									\
> +      td_err_e __err = tdb_testinfo->info->func ## _p (args);		\
> +									\
> +      __CHECK (__err == TD_OK, _("%s failed: %s"), #func,		\
> +	       thread_db_err_str (__err));				\

Likewise __err.

> +# 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 test only works for native processes on Linux.

GNU/Linux, because glibc.

> +if { ![isnative] || [is_remote host] || [target_info exists use_gdb_stub]
> +     || ![istarget *-linux*] } {
> +    continue

I think this fails to filter out --target_board=native-extended-gdbserver ?

This would be more to the point, and would work, I think (haven't tested):

# This test only works for native processes on GNU/Linux.
if {[target_info gdb_protocol] != "" || ![istarget *-linux*]} {
   continue
}


> +}
> +
> +standard_testfile
> +
> +if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
> +	 executable debug] != "" } {
> +    return -1
> +}
> +
> +# TEST 1: Manual check with libthread_db not loaded.

Please remove the numbering in "TEST 1", etc.  We have a few old
testcases that have comments like that, and over the years as tests
are added/removed to the file, the numbering gets annoyingly in the
way / odd, when inevitably in the future we want to insert a
test in the middle, remove or reorder tests, etc.

> +
> +clean_restart ${binfile}
> +
> +gdb_test "maint show check-libthread-db" \
> +    "Whether to check libthread_db at load time is off."
> +
> +gdb_test_no_output "set stop-on-solib-events 1"
> +gdb_run_cmd
> +gdb_test "" \
> +    ".*Stopped due to shared library event.*no libraries added or removed.*"
> +
> +gdb_test "maint check libthread-db" \
> +    "No libthread_db loaded" \
> +    "libpthread.so not loaded"
> +
> +
> +# TEST 2: Manual check with NPTL uninitialized.
> +# libthread_db should fake a single thread with th_unique == NULL.
> +
> +gdb_test "continue" \
> +    ".*Stopped due to shared library event.*Inferior loaded .*libpthread.*"
> +
> +gdb_test_sequence "maint check libthread-db" \
> +    "libpthread.so not initialized (manual)" {
> +	"\[\r\n\]+Running libthread_db integrity checks:"
> +	"\[\r\n\]+\[ \]+Got thread 0x0 => \[0-9\]+ => 0x0 ... OK"
> +	"\[\r\n\]+libthread_db integrity checks passed."
> +    }
> +
> +
> +# TEST 3: Manual check with NPTL fully operational.
> +
> +gdb_test_no_output "set stop-on-solib-events 0"
> +gdb_breakpoint break_here
> +gdb_continue_to_breakpoint break_here
> +
> +gdb_test_sequence "maint check libthread-db" \
> +    "libpthread.so fully initialized (manual)" {
> +	"\[\r\n\]+Running libthread_db integrity checks:"
> +	"\[\r\n\]+\[ \]+Got thread 0x\[1-9a-f\]\[0-9a-f\]+ => \[0-9\]+ => 0x\[1-9a-f\]\[0-9a-f\]+; errno = 23 ... OK"
> +	"\[\r\n\]+\[ \]+Got thread 0x\[1-9a-f\]\[0-9a-f\]+ => \[0-9\]+ => 0x\[1-9a-f\]\[0-9a-f\]+; errno = 42 ... OK"
> +	"\[\r\n\]+libthread_db integrity checks passed."
> +    }
> +
> +
> +# TEST 4: Automated check with NPTL uninitialized.
> +
> +clean_restart ${binfile}
> +
> +gdb_test_no_output "maint set check-libthread-db 1"
> +gdb_test_no_output "set debug libthread-db 1"
> +gdb_breakpoint break_here
> +gdb_run_cmd
> +
> +gdb_test_sequence "" \
> +    "libpthread.so not initialized (automated)" {
> +	"\[\r\n\]+Running libthread_db integrity checks:"
> +	"\[\r\n\]+\[ \]+Got thread 0x0 => \[0-9\]+ => 0x0 ... OK"
> +	"\[\r\n\]+libthread_db integrity checks passed."
> +	"\[\r\n\]+[Thread debugging using libthread_db enabled]"
> +    }
> +
> +
> +# TEST 5: Automated check with NPTL fully operational.
> +
> +clean_restart ${binfile}
> +
> +gdb_test_no_output "maint set check-libthread-db 1"
> +gdb_test_no_output "set debug libthread-db 1"
> +
> +set test_spawn_id [spawn_wait_for_attach $binfile]
> +set testpid [spawn_id_get_pid $test_spawn_id]
> +
> +gdb_test_sequence "attach $testpid" \
> +    "libpthread.so fully initialized (automated)" {
> +	"\[\r\n\]+Running libthread_db integrity checks:"
> +	"\[\r\n\]+\[ \]+Got thread 0x\[1-9a-f\]\[0-9a-f\]+ => \[0-9\]+ => 0x\[1-9a-f\]\[0-9a-f\]+ ... OK"
> +	"\[\r\n\]+\[ \]+Got thread 0x\[1-9a-f\]\[0-9a-f\]+ => \[0-9\]+ => 0x\[1-9a-f\]\[0-9a-f\]+ ... OK"
> +	"\[\r\n\]+libthread_db integrity checks passed."
> +	"\[\r\n\]+[Thread debugging using libthread_db enabled]"
> +    }
> +
> +
> +gdb_exit
> +kill_wait_spawned_process $test_spawn_id
> 

I'm surprised to not see any validation when debugging
a core dump.  Would it work?

Thanks,
Pedro Alves