Re: [PATCH 1/2 ver 2] Fix reverse stepping multiple contiguous PC ranges over the line table.

[Guinevere Larsen <blarsen@redhat.com>]

On 08/08/2023 17:38, Carl Love wrote:
> Guinevere:
>
> On Tue, 2023-08-08 at 12:04 +0200, Guinevere Larsen wrote:
> <snip>
>
>> On 07/08/2023 21:03, Carl Love wrote:
>>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>>> index 36bf738c667..bffbbf38b09 100644
>>> --- a/gdb/testsuite/lib/gdb.exp
>>> +++ b/gdb/testsuite/lib/gdb.exp
>>> @@ -4896,6 +4896,8 @@ proc quote_for_host { args } {
>>>    #     debug information
>>>    #   - text_segment=addr: Tell the linker to place the text
>>> segment at ADDR.
>>>    #   - build-id: Ensure the final binary includes a build-id.
>>> +#   - no-column-info: Disable generation of column table
>>> information.
>>> +#   - column-info: Enable generation of column table information.
>>>    #
>>>    # And here are some of the not too obscure options understood by
>>> DejaGnu that
>>>    # influence the compilation:
>>> @@ -5105,6 +5107,38 @@ proc gdb_compile {source dest type options}
>>> {
>>>                } else {
>>>                    error "Don't know how to handle text_segment
>>> option."
>>>                }
>>> +	} elseif { $opt == "column-info" } {
>>> +	    # If GCC or clang does not support column-info, compilation
>>> +	    # will fail and the usupported column-info option will be
>>> +	    # reported as such.
>>> +	    if {[test_compiler_info {gcc-*}]} {
>> I think you missed a bit on an old comment from simon. Way back in
>> may,
>> in this email
>> https://sourceware.org/pipermail/gdb-patches/2023-May/199523.html ,
>> he
>> mentioned:
>>
>> For instance, if you used no-column-info with gcc 6
>> (which doesn't support column info at all), gdb_compile should
>> succeed,
>> even if there isn't an option to disable column info with that
>> compiler.
>> If you used column-info with gcc 6, gdb_compile would fail.
>>
>> So I think this bit should throw an error if it detects gcc-[1-6].
> It has been awhile, but as I recall, we decided that the we would
> specify column-info and if the compiler doesn't support it then the
> compiler will complain (i.e. fail) and we will let the failure be
> handled by the normal compiler failure path.  I think that will work
> fine?  If there is some concern that is not sufficient, I would be
> happy to put in the test   if {[test_compiler_info {gcc-[1-6]-*}]}  for
> the $opt == "column-info"  to have the script flag the error.
> Thoughts?

oh, I see. I must have misread when looking back at that conversation. 
Sorry for the noise.

Also, disregard my comments about LLVM. I checked on IRC and the oldest 
GCC we support compiling/using for usptream stuff is form 2014, so I 
think we dont have to worry about 2012 clang :)

All in all, this patch looks good to go in. Reviewed-By: Guinevere 
Larsen <blarsen@redhat.com>

I hope the maintainers approve this series soon, it is a long time coming!

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>
> In the case where the compiler doesn't handle the no-column-info flag,
> i.e. gcc 1-6, we handle that case by not adding the flag so the
> compiler will not flag the error and fail.  In that case, it isn't
> going to generate the column info anyways so we don't need to specify
> no-column info.
>
>
>>> +		lappend new_options "additional_flags=-gcolumn-info"
>>> +
>>> +	    } elseif {[test_compiler_info {clang-*}]} {
>> I did some digging, and column-info were added in llvm back in
>> october
>> 2012 (commit a2f7eb7c52cdc), which seems to mean support was added
>> in
>> llvm 3.2, but I don't see any mention in the release notes. In my
>> opinion, this is old enough that we don't need to have a special
>> case,
>> but I wanted to mention, in case some maintainer thinks it should be
>> dealt with.
>>
>> If we should, before then, it seems that clang WOULD add column info
>> by
>> default, so it should compile with a warning here, and fail if the
>> user
>> requested no column info
>>
>                                               Carl
>