[PINGv2][PATCH v3] gdb: register frame_destroyed function for amd64 gdbarch

[Guinevere Larsen <blarsen@redhat.com>]

Ping!
On 07/12/2023 18:36, Guinevere Larsen wrote:
> Ping!
>
> -- 
> Cheers,
> Guinevere Larsen
> She/Her/Hers
>
> On 08/11/2023 15:24, Guinevere Larsen wrote:
>> gdbarches usually register functions to check when a frame is destroyed
>> which is used with software watchpoints, since the expression of the
>> watchpoint is no longer vlaid at this point.  On amd64, this wasn't done
>> anymore because GCC started using CFA for variable locations instead.
>>
>> However, clang doesn't use the CFA and instead relies on specifying when
>> an epilogue has started, meaning software watchpoints get a spurious hit
>> when a frame is destroyed. This patch re-adds the code to register the
>> function that detects when a frame is destroyed, but only uses this when
>> the producer is LLVM, so gcc code isn't affected. The logic that
>> identifies the epilogue has been factored out into the new function
>> amd64_stack_frame_destroyed_p_1, so the frame sniffer can call it
>> directly, and its behavior isn't changed.
>>
>> This can also remove the XFAIL added to gdb.python/pq-watchpoint tests
>> that handled this exact flaw in clang
>>
>> Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
>> ---
>>   gdb/amd64-tdep.c                           | 32 ++++++++++++++++------
>>   gdb/testsuite/gdb.python/py-watchpoint.exp | 17 +-----------
>>   2 files changed, 25 insertions(+), 24 deletions(-)
>>
>> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
>> index e6feee677b3..2e101b4fca1 100644
>> --- a/gdb/amd64-tdep.c
>> +++ b/gdb/amd64-tdep.c
>> @@ -2886,6 +2886,22 @@ static const struct frame_base amd64_frame_base =
>>     amd64_frame_base_address
>>   };
>>   +/* Implement core of the stack_frame_destroyed_p gdbarch method.  */
>> +
>> +static int
>> +amd64_stack_frame_destroyed_p_1 (struct gdbarch *gdbarch, CORE_ADDR pc)
>> +{
>> +  gdb_byte insn;
>> +
>> +  if (target_read_memory (pc, &insn, 1))
>> +    return 0;   /* Can't read memory at pc.  */
>> +
>> +  if (insn != 0xc3)     /* 'ret' instruction.  */
>> +    return 0;
>> +
>> +  return 1;
>> +}
>> +
>>   /* Normal frames, but in a function epilogue.  */
>>     /* Implement the stack_frame_destroyed_p gdbarch method.
>> @@ -2897,15 +2913,13 @@ static const struct frame_base 
>> amd64_frame_base =
>>   static int
>>   amd64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
>>   {
>> -  gdb_byte insn;
>> +  struct compunit_symtab *cust = find_pc_compunit_symtab (pc);
>>   -  if (target_read_memory (pc, &insn, 1))
>> -    return 0;   /* Can't read memory at pc.  */
>> +  if (cust != nullptr && cust->producer () != nullptr
>> +      && producer_is_llvm (cust->producer ()))
>> +    return amd64_stack_frame_destroyed_p_1 (gdbarch, pc);
>>   -  if (insn != 0xc3)     /* 'ret' instruction.  */
>> -    return 0;
>> -
>> -  return 1;
>> +  return 0;
>>   }
>>     static int
>> @@ -2938,7 +2952,7 @@ amd64_epilogue_frame_sniffer_1 (const struct 
>> frame_unwind *self,
>>       }
>>       /* Check whether we're in an epilogue.  */
>> -  return amd64_stack_frame_destroyed_p (gdbarch, pc);
>> +  return amd64_stack_frame_destroyed_p_1 (gdbarch, pc);
>>   }
>>     static int
>> @@ -3310,6 +3324,8 @@ amd64_init_abi (struct gdbarch_info info, 
>> struct gdbarch *gdbarch,
>>       set_gdbarch_gen_return_address (gdbarch, 
>> amd64_gen_return_address);
>>   +  set_gdbarch_stack_frame_destroyed_p (gdbarch, 
>> amd64_stack_frame_destroyed_p);
>> +
>>     /* SystemTap variables and functions.  */
>>     set_gdbarch_stap_integer_prefixes (gdbarch, stap_integer_prefixes);
>>     set_gdbarch_stap_register_prefixes (gdbarch, 
>> stap_register_prefixes);
>> diff --git a/gdb/testsuite/gdb.python/py-watchpoint.exp 
>> b/gdb/testsuite/gdb.python/py-watchpoint.exp
>> index 5ff61285979..9a6ef447572 100644
>> --- a/gdb/testsuite/gdb.python/py-watchpoint.exp
>> +++ b/gdb/testsuite/gdb.python/py-watchpoint.exp
>> @@ -42,20 +42,5 @@ gdb_test "source $pyfile" ".*Python script 
>> imported.*" \
>>       "import python scripts"
>>   gdb_test "python print(len(gdb.breakpoints()))" "2" "check modified 
>> BP count"
>>   gdb_test "continue" ".*" "run until program stops"
>> -# Clang doesn't use CFA location information for variables (despite 
>> generating
>> -# them), meaning when the instruction "pop rbp" happens, we get a 
>> false hit
>> -# on the watchpoint. for more details, see:
>> -# https://github.com/llvm/llvm-project/issues/64390
>> -gdb_test_multiple "python print(bpt.n)" "check watchpoint hits" {
>> -    -re -wrap "5" {
>> -    pass $gdb_test_name
>> -    }
>> -    -re -wrap "6" {
>> -    if {[test_compiler_info "clang-*"]} {
>> -        xfail "$gdb_test_name (clang issue 64390)"
>> -    } else {
>> -        fail $gdb_test_name
>> -    }
>> -    }
>> -}
>> +gdb_test "python print(bpt.n)" "5" "check watchpoint hits"
>>   gdb_test "python print(len(gdb.breakpoints()))" "1" "check BP count" 


-- 
Cheers,
Guinevere Larsen
She/Her/Hers