Re: [PATCH v4] gdb/gcore: interrupt all threads before generating the corefile

[Lancelot SIX <Lancelot.Six@amd.com>]

Hi,

Thanks for the review!

>> +  bool all_stop_was_running = false;
>> +  if (exists_non_stop_target ())
>> +    stop_all_threads ("generating coredump", inf);
>> +  else
>> +    {
>> +      all_stop_was_running = any_thread_of_inferior (inf)->executing ();
> I'm wondering what happens if you call this while the inferior is doing
> an in-line step over, with one thread resumed and the others stopped.  I
> think the others will have executing == false, so could you fall on one
> that isn't executing?

I am not entirely sure about those cases, but you might be right.  I 
could explicitly check all threads:

for (thread_info *t : all_non_exited_threads (inf->process_target (),
                                               ptid_t (inf->pid))
   all_stop_was_running |= t->executing ();

I'll do that in next revision.

>> +
>> +# This testcase checks that when in non-stop mode with some threads running
>> +# the gcore command can interrupt all threads, generate a core dump and
>> +# restart threads as required.
>> +
> 
> Please add a short comment explaining the goal of this test.
> 

There is one just above, but I recon it can be improved.

>> +standard_testfile
>> +
>> +if { [prepare_for_testing "failed to prepare" \
>> +     ${testfile} ${srcfile} {pthreads debug}] } {
>> +    return
>> +}
> 
> prepare_for_testing starts GDB, but it gets restarted below anyway.  Use
> build_executable to avoid the unnecessary spawn.
> 

I will do that for next revision.

>> +
>> +save_vars { GDBFLAGS } {
>> +    append GDBFLAGS " -ex \"set non-stop on\""
>> +    clean_restart ${binfile}
>> +}
>> +
>> +set lineno [gdb_get_line_number "Break here"]
>> +if { ![runto $lineno] } {
>> +    return
>> +}
>> +
>> +# We should be stopped in thread 1 while thread 2 is running.
>> +gdb_test_sequence "info threads" "info threads" {
>> +    {Id\s+Target Id\s+Frame}
>> +    {\*\s+1[^\n]*\n}
>> +    {\s+2\s+[^\n]*\(running\)[^\n]*\n}
>> +}
>> +
>> +set th1_pc ""
>> +gdb_test_multiple "p/x \$pc" "fetch thread 1 PC" {
>> +  -wrap -re "$::decimal = ($::hex)" {
>> +    set th1_pc $expect_out(1,string)
>> +    pass $gdb_test_name
>> +  }
> 
> Could that use get_hexadecimal_valueof?
> 

Probably yes.  I will use this also.

> Simon
> 

Thanks,
Lancelot.