[Bug tdep/29804] New: [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[Bug tdep/29804] New: [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29804

            Bug ID: 29804
           Summary: [gdb/tdep, vax] Remove
                    gdbarch_deprecated_function_start_offset
           Product: gdb
           Version: HEAD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: tdep
          Assignee: unassigned at sourceware dot org
          Reporter: vries at gcc dot gnu.org
  Target Milestone: ---

As noted in PR29793, there's only one user left of
gdbarch_deprecated_function_start_offset:
...
$ find gdb -type f | xargs grep set_gdbarch_deprecated_function_start_offset
gdb/gdbarch-gen.h:extern void set_gdbarch_deprecated_function_start_offset
(struct gdbarch *gdbarch, CORE_ADDR deprecated_function_start_offset);
gdb/vax-tdep.c:  set_gdbarch_deprecated_function_start_offset (gdbarch, 2);
gdb/gdbarch.c:set_gdbarch_deprecated_function_start_offset (struct gdbarch
*gdbarch,
...

The vax target.

The deprecation started in 2004:
...
2004-06-18  Andrew Cagney  <cagney@gnu.org>

        * gdbarch.sh (DEPRECATED_FUNCTION_START_OFFSET): Deprecated.
...

The submission email (
https://sourceware.org/pipermail/gdb-patches/2004-May/034563.html ) mentions
vax:
...
The VAX uses this to ``adjust'' a function 
pointer so that it instead points to the function's code address (the 
first few bytes of a vax function are not executable and need to be 
skipped over).

Thing is, this conversion is exactly what CONVERT_FUNC_PTR_ADDR is for. 
  Given a function pointer return the function's code address.  Given 
this overlap, I think FUNCTION_START_OFFSET should be deprecated - it is 
redundant.
...

Unfortunately grepping through the sources shows that code locations of 
gdbarch_deprecated_function_start_offset and convert_from_func_ptr_addr don't
always overlap, so this is not a mechanical substitution, and needs to be
tested.

One outcome could be that we cannot (fully) remove it, but then we should
de-deprecate it and document precisely why we need it and when and where this
hook is necessary.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/29804] [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29804

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |macro at orcam dot me.uk

--- Comment #1 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Tom de Vries from comment #0)
> Unfortunately grepping through the sources shows that code locations of 
> gdbarch_deprecated_function_start_offset and convert_from_func_ptr_addr
> don't always overlap, so this is not a mechanical substitution, and needs to
> be tested.

Maciej,

I saw that when vax was announced to be deprecated as gcc target due to using
cc0, you did the conversion from cc0 to MODE_CC.

Are you able to test on vax?

Are you willing to look into this PR?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/29804] [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29804

--- Comment #2 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Tom de Vries from comment #0)
> Thing is, this conversion is exactly what CONVERT_FUNC_PTR_ADDR is for. 

It's debatable whether we should use convert_from_func_ptr_addr or
gdbarch_skip_entrypoint.

In practise the determining factor for which to use could be just where the two
are used in the code.

Indeed the description of convert_from_func_ptr_addr (as confusingly given in
the comment for gdbarch_deprecated_function_start_offset) seems to fit nicely:
...
/* A function can be addressed by either it's "pointer" (possibly a             
   descriptor address) or "entry point" (first executable instruction).         
   The method "convert_from_func_ptr_addr" converting the former to the         
   latter.  gdbarch_deprecated_function_start_offset is being used to implement 
   a simplified subset of that functionality - the function's address           
   corresponds to the "function pointer" and the function's start               
   corresponds to the "function entry point" - and hence is redundant. */
...

But I also saw in the gcc backend:
...
/* Output code to add DELTA to the first argument, and then jump to FUNCTION.   
   Used for C++ multiple inheritance.                                           
        .mask   ^m<r2,r3,r4,r5,r6,r7,r8,r9,r10,r11>  #conservative entry mask   
        addl2   $DELTA, 4(ap)   #adjust first argument                          
        jmp     FUNCTION+2      #jump beyond FUNCTION's entry mask             
              ...
So, we could describe this situation by saying there are two entry points for
FUNCTION:
- one for calls (target address at mask), and
- one for jumps (target address past mask).

Indeed

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/29804] [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29804

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tromey at sourceware dot org

--- Comment #3 from Tom Tromey <tromey at sourceware dot org> ---
If we had a VAX executable, would that be enough to
test any proposed change?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/29804] [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29804

--- Comment #4 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Tom Tromey from comment #3)
> If we had a VAX executable, would that be enough to
> test any proposed change?

I'm not sure I understand your question, possibly I'm missing some context
related to vax, but my understanding is that in order to test a potential patch
we'd need to be able to run the gdb testsuite on vax without and with the
patch, to make sure we don't introduce regressions.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/29804] [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[macro at orcam dot me.uk]

https://sourceware.org/bugzilla/show_bug.cgi?id=29804

--- Comment #5 from Maciej W. Rozycki <macro at orcam dot me.uk> ---
(In reply to Tom de Vries from comment #1)
> Maciej,
> 
> I saw that when vax was announced to be deprecated as gcc target due to
> using cc0, you did the conversion from cc0 to MODE_CC.
> 
> Are you able to test on vax?
> 
> Are you willing to look into this PR?
My VAX box is up and running, however I note that we have no `gdbserver'
for VAX/NetBSD and I have no TCL/Expect/DejaGNU installed there.  Native
testing would take forever anyway.  Given that we do have NetBSD backends
in `gdbserver' already perhaps it'll be the easiest if we add support to
the VAX target there.  I'll see what I can do, but it may take a while.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/29804] [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29804

--- Comment #6 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Maciej W. Rozycki from comment #5)
> My VAX box is up and running, however I note that we have no `gdbserver'
> for VAX/NetBSD and I have no TCL/Expect/DejaGNU installed there.  Native
> testing would take forever anyway.  Given that we do have NetBSD backends
> in `gdbserver' already perhaps it'll be the easiest if we add support to
> the VAX target there.  I'll see what I can do, but it may take a while.

Thanks for looking into this.

A setup that might also work and doesn't require gdbserver is remote
host+target, which runs tcl/expect/dejagnu on a some local machine (build
role), and runs compiler, gdb and target execs on a remote machine (host/target
role).  But I suppose it would run into the same "would take forever" problem
as native testing.

It all makes me wonder how this target used to be validated.  Maybe done in a
time when the testsuite was smaller, and took significantly less time to
execute. Or perhaps a subset of the testsuite was taken and deemed sufficient.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/29804] [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[macro at orcam dot me.uk]

https://sourceware.org/bugzilla/show_bug.cgi?id=29804

--- Comment #7 from Maciej W. Rozycki <macro at orcam dot me.uk> ---
I could install the required packages, but I don't feel like cluttering
the system for what I think makes little sense.  I only have 650MB of
free local storage available (out of 2GB total) too, which means there's
no local space for a single GDB build even and it all would have to run
over NFS, making things even slower.

I suspect this target may not have been validated for a long time and
things certainly used to be more suited performance-wise to hardware of
the time decades ago.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/29804] [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29804

--- Comment #8 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Maciej W. Rozycki from comment #7)
> I could install the required packages, but I don't feel like cluttering
> the system for what I think makes little sense.  I only have 650MB of
> free local storage available (out of 2GB total) too, which means there's
> no local space for a single GDB build even and it all would have to run
> over NFS, making things even slower.
> 

Ack, understood.  I then wonder if it's possible to do a local cross build for
make all-gdb, and use the artefacts to do a make check-gdb using remote
host+target somehow, but perhaps that'll take more work than to get gdbserver
working.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/29804] [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[macro at orcam dot me.uk]

https://sourceware.org/bugzilla/show_bug.cgi?id=29804

--- Comment #9 from Maciej W. Rozycki <macro at orcam dot me.uk> ---
Anything but writing `gdbserver' would be a one-off effort that would
have to be repeated next time in the future.  So even if it was cheaper
effort-wise, it would be less profitable long-term.  Besides, I wrote the
RISC-V/Linux backend a couple of years back and it was already
straightforward and VAX is even simpler to handle with its single unified
32-bit user register file shared by all implementations (and no prospect
to get the vector unit supported ever).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/29804] [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29804

--- Comment #10 from Tom Tromey <tromey at sourceware dot org> ---
(In reply to Tom de Vries from comment #4)
> (In reply to Tom Tromey from comment #3)
> > If we had a VAX executable, would that be enough to
> > test any proposed change?
> 
> I'm not sure I understand your question, possibly I'm missing some context
> related to vax, but my understanding is that in order to test a potential
> patch we'd need to be able to run the gdb testsuite on vax without and with
> the patch, to make sure we don't introduce regressions.

Sorry, what I meant is that for a reasonably obscure target like
this, and for a change that just affects some prologue skipping,
it seems to me that we could test it with just an executable
and by examining the effects of "break whatever".

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/29804] [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[macro at orcam dot me.uk]

https://sourceware.org/bugzilla/show_bug.cgi?id=29804

--- Comment #11 from Maciej W. Rozycki <macro at orcam dot me.uk> ---
(In reply to Tom Tromey from comment #10)
> > I'm not sure I understand your question, possibly I'm missing some context
> > related to vax, but my understanding is that in order to test a potential
> > patch we'd need to be able to run the gdb testsuite on vax without and with
> > the patch, to make sure we don't introduce regressions.
> 
> Sorry, what I meant is that for a reasonably obscure target like
> this, and for a change that just affects some prologue skipping,
> it seems to me that we could test it with just an executable
> and by examining the effects of "break whatever".
Well, I'm perfectly happy to regression-test this target, whether for
this PR or overall to know how it performs (or does not).  I have it
scheduled later this month.  If that does not work for some reason,
then we can look for a fallback solution.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug tdep/29804] [gdb/tdep, vax] Remove gdbarch_deprecated_function_start_offset

[macro at orcam dot me.uk]

https://sourceware.org/bugzilla/show_bug.cgi?id=29804

--- Comment #12 from Maciej W. Rozycki <macro at orcam dot me.uk> ---
For the record, I have VAX/NetBSD `gdbserver' implemented however due to
non-functional exception handling with this configuration it always
crashes upon exit with:

terminate called after throwing an instance of 'gdb_exception_quit'
terminate called recursively
Abort

Similarly does native GDB as included with the distribution, e.g.:

(gdb) ^Cterminate called after throwing an instance of
'gdb_exception_RETURN_MASK_QUIT'
terminate called recursively
Abort

I have assessed the problem and concluded that the generic DWARF unwinder
included with libgcc is unsuitable as it stands for the VAX frame layout,
because it makes too many assumptions that are wrong for the VAX ABI.
Most importantly it thinks that it needs to update $sp to unwind a frame,
but it has to be $fp with the VAX ABI ($sp only really matters for
outgoing function arguments and alloca, $ap is used for incoming function
arguments, and $fp is used for everything else).  And in any case DWARF
EH records produced by GCC for the VAX target are broken in many ways.
AFAICT exception handling has never worked for the ELF VAX/NetBSD target
(perhaps it worked for a.out).

With the VAX ABI however there is full frame information always available
(the RET machine instruction makes use of it to restore all the static
registers saved by the CALLS instruction and move to the previous frame)
and one does not need to resort to DWARF records to be able to refer to
arbitrary frames (perhaps except for signal frames; I have not checked
that yet).  Therefore I have not decided yet whether the solution for
exception handling will be a VAX-specific implementation of a DWARF
unwinder or a completely different VAX-specific unwinder working directly
on the frame structure itself.  I do hope to get it sorted with GCC 14
later this year.

Anyway this implementation of `gdbserver' is otherwise fully functional
and has turned out good enough to figure out what is going on with frame
unwinding, so I'll see if it is suitable to run regression testing
despite of crashing upon exit and will submit it anyway.  There is a
change or two required to GDB itself to make the testsuite work with the
generic core of NetBSD `gdbserver', which makes me suspect remote
regression testing has never ever been run with any NetBSD target.

-- 
You are receiving this mail because:
You are on the CC list for the bug.