Re: [PUSHED] sim: fixes for libopcodes styled disassembler

[Andrew Burgess <aburgess@redhat.com>]

* Jan-Benedict Glaw <jbglaw@lug-owl.de> [2022-09-03 08:57:32 +0200]:

> On Mon, 2022-04-04 22:46:32 +0100, Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> wrote:
> > I've gone ahead and pushed the patch below without review, despite it
> > being non-trivial.  I stupidly managed to break several sim targets,
> > and I wanted to undo the breakage asap.
> > 
> > Despite being quite large, I hope nothing in here is too
> > controversial.  However, if there is any feedback, do please leave it
> > and I'll be happy to fix any issues in a follow up patch.
> > 
> > Sorry for the breakage, and thank you for your understanding.
> 
> .../configure --prefix=... --target=sparc-linux
> make V=1 all-gdb
> 
> [...]
> rm -f gdb
> g++  -g -O2       -pthread  \
>         -o gdb gdb.o ada-exp.o ada-lang.o ada-tasks.o ada-typeprint.o ada-valprint.o ada-varobj.o addrmap.o agent.o alloc.o annotate.o arch-utils.o async-event.o auto-load.o auxv.o ax-gdb.o ax-general.o bcache.o bfd-target.o block.o blockframe.o break-catch-exec.o break-catch-fork.o break-catch-load.o break-catch-sig.o break-catch-syscall.o break-catch-throw.o breakpoint.o bt-utils.o btrace.o build-id.o buildsym-legacy.o buildsym.o c-exp.o c-lang.o c-typeprint.o c-valprint.o c-varobj.o charset.o cli-out.o cli/cli-cmds.o cli/cli-decode.o cli/cli-dump.o cli/cli-interp.o cli/cli-logging.o cli/cli-option.o cli/cli-script.o cli/cli-setshow.o cli/cli-style.o cli/cli-utils.o coff-pe-read.o coffread.o compile/compile-c-support.o compile/compile-c-symbols.o compile/compile-c-types.o compile/compile-cplus-symbols.o compile/compile-cplus-types.o compile/compile-loc2c.o compile/compile-object-load.o compile/compile-object-run.o compile/compile.o complaints.o completer.o copying.o corefile.o corelow.o cp-abi.o cp-name-parser.o cp-namespace.o cp-support.o cp-valprint.o ctfread.o d-exp.o d-lang.o d-namespace.o d-valprint.o dbxread.o dcache.o debug.o debuginfod-support.o dictionary.o disasm-selftests.o disasm.o displaced-stepping.o dtrace-probe.o dummy-frame.o dwarf2/abbrev-cache.o dwarf2/abbrev.o dwarf2/attribute.o dwarf2/comp-unit-head.o dwarf2/cooked-index.o dwarf2/cu.o dwarf2/dwz.o dwarf2/expr.o dwarf2/frame-tailcall.o dwarf2/frame.o dwarf2/index-cache.o dwarf2/index-common.o dwarf2/index-write.o dwarf2/leb.o dwarf2/line-header.o dwarf2/loc.o dwarf2/macro.o dwarf2/read.o dwarf2/section.o dwarf2/stringify.o elf-none-tdep.o elfread.o eval.o event-top.o exceptions.o exec.o expprint.o extension.o f-exp.o f-lang.o f-typeprint.o f-valprint.o filename-seen-cache.o filesystem.o findcmd.o findvar.o frame-base.o frame-unwind.o frame.o gcore-elf.o gcore.o gdb-demangle.o gdb_bfd.o gdbarch-selftests.o gdbtypes.o gmp-utils.o gnu-v2-abi.o gnu-v3-abi.o go-exp.o go-lang.o go-typeprint.o go-valprint.o guile/guile.o inf-child.o inf-loop.o infcall.o infcmd.o inferior.o inflow.o infrun.o inline-frame.o interps.o jit.o language.o linespec.o linux-tdep.o location.o m2-exp.o m2-lang.o m2-typeprint.o m2-valprint.o macrocmd.o macroexp.o macroscope.o macrotab.o main.o maint-test-options.o maint-test-settings.o maint.o mdebugread.o mem-break.o memattr.o memory-map.o memrange.o memtag.o mi/mi-cmd-break.o mi/mi-cmd-catch.o mi/mi-cmd-disas.o mi/mi-cmd-env.o mi/mi-cmd-file.o mi/mi-cmd-info.o mi/mi-cmd-stack.o mi/mi-cmd-target.o mi/mi-cmd-var.o mi/mi-cmds.o mi/mi-common.o mi/mi-console.o mi/mi-getopt.o mi/mi-interp.o mi/mi-main.o mi/mi-out.o mi/mi-parse.o mi/mi-symbol-cmds.o minidebug.o minsyms.o mipsread.o namespace.o objc-lang.o objfiles.o observable.o opencl-lang.o osabi.o osdata.o p-exp.o p-lang.o p-typeprint.o p-valprint.o parse.o posix-hdep.o printcmd.o probe.o process-stratum-target.o producer.o progspace-and-thread.o progspace.o prologue-value.o psymtab.o python/python.o ravenscar-thread.o record-btrace.o record-full.o record.o regcache-dump.o regcache.o reggroups.o remote-fileio.o remote-notif.o remote-sim.o remote.o reverse.o run-on-main-thread.o rust-lang.o rust-parse.o selftest-arch.o sentinel-frame.o ser-base.o ser-event.o ser-pipe.o ser-tcp.o ser-uds.o ser-unix.o serial.o skip.o solib-svr4.o solib-target.o solib.o source-cache.o source.o sparc-linux-tdep.o sparc-ravenscar-thread.o sparc-tdep.o sparc64-linux-tdep.o sparc64-tdep.o split-name.o stabsread.o stack.o stap-probe.o std-regs.o symfile-debug.o symfile-mem.o symfile.o symmisc.o symtab.o target-connection.o target-dcache.o target-descriptions.o target-float.o target-memory.o target.o target/target.o target/waitstatus.o test-target.o thread-iter.o thread.o tid-parse.o top.o tracectf.o tracefile-tfile.o tracefile.o tracepoint.o trad-frame.o tramp-frame.o tui/tui-command.o tui/tui-data.o tui/tui-disasm.o tui/tui-file.o tui/tui-hooks.o tui/tui-interp.o tui/tui-io.o tui/tui-layout.o tui/tui-location.o tui/tui-out.o tui/tui-regs.o tui/tui-source.o tui/tui-stack.o tui/tui-win.o tui/tui-wingeneral.o tui/tui-winsource.o tui/tui.o type-stack.o typeprint.o ui-file.o ui-out.o ui-style.o unittests/array-view-selftests.o unittests/child-path-selftests.o unittests/cli-utils-selftests.o unittests/command-def-selftests.o unittests/common-utils-selftests.o unittests/copy_bitwise-selftests.o unittests/enum-flags-selftests.o unittests/environ-selftests.o unittests/filtered_iterator-selftests.o unittests/format_pieces-selftests.o unittests/function-view-selftests.o unittests/gdb_tilde_expand-selftests.o unittests/gmp-utils-selftests.o unittests/intrusive_list-selftests.o unittests/lookup_name_info-selftests.o unittests/main-thread-selftests.o unittests/memory-map-selftests.o unittests/memrange-selftests.o unittests/mkdir-recursive-selftests.o unittests/observable-selftests.o unittests/offset-type-selftests.o unittests/optional-selftests.o unittests/packed-selftests.o unittests/parallel-for-selftests.o unittests/parse-connection-spec-selftests.o unittests/path-join-selftests.o unittests/ptid-selftests.o unittests/rsp-low-selftests.o unittests/scoped_fd-selftests.o unittests/scoped_ignore_signal-selftests.o unittests/scoped_mmap-selftests.o unittests/scoped_restore-selftests.o unittests/search-memory-selftests.o unittests/string_view-selftests.o unittests/style-selftests.o unittests/tracepoint-selftests.o unittests/tui-selftests.o unittests/ui-file-selftests.o unittests/unique_xmalloc_ptr_char.o unittests/unpack-selftests.o unittests/utils-selftests.o unittests/vec-utils-selftests.o unittests/xml-utils-selftests.o user-regs.o utils.o valarith.o valops.o valprint.o value.o varobj.o version.o xml-builtin.o xml-support.o xml-syscall.o xml-tdesc.o init.o \
>           ../sim/erc32/libsim.a ../readline/readline/libreadline.a ../opcodes/libopcodes.a ../libctf/.libs/libctf.a ../bfd/libbfd.a -L./../zlib -lz ../gdbsupport/libgdbsupport.a  ../libiberty/libiberty.a ../libdecnumber/libdecnumber.a    ../libbacktrace/.libs/libbacktrace.a -lncursesw -lm -ldl      ../gnulib/import/libgnu.a    -lgmp      
> /usr/bin/ld: ../sim/erc32/libsim.a(interf.o): in function `sim_open':
> /var/cache/git/binutils-gdb/sim/erc32/interf.c:247: undefined reference to `fprintf_styled'
> collect2: error: ld returned 1 exit status
> make[1]: *** [Makefile:2142: gdb] Error 1
> make[1]: Leaving directory '/tmp/sparc/b/gdb'
> make: *** [Makefile:13193: all-gdb] Error 2
> 
> 
> erc32 has two INIT_DISASSEMBLE_INFO(), of which one doesn't pick the
> provided function. Maybe just ordering?

Thanks for pointing this out.

I pushed the patch below which should fix this problem.   Let me know
if you still have any problems with the build.

Thanks,
Andrew

---

commit a411a714f31da81e6c57317f1d392e166053dff1
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sun Sep 4 17:49:11 2022 +0100

    sim/erc32: fix gdb with simulator build
    
    In commit:
    
      commit 7b01c1cc1d111ba0afa51e60fa9842d3b971e2d1
      Date:   Mon Apr 4 22:38:04 2022 +0100
    
          sim: fixes for libopcodes styled disassembler
    
    changes were made to the simulator source to handle the new libopcodes
    disassembler styling API.
    
    Unfortunately, these changes broke building GDB with the erc32 (sparc)
    simulator, like this:
    
      ../src/configure --target=sparc-linux
      make all-gdb
      ....
      /usr/bin/ld: ../sim/erc32/libsim.a(interf.o): in function `sim_open':
      /tmp/build/sim/../../src/sim/erc32/interf.c:247: undefined reference to `fprintf_styled'
      collect2: error: ld returned 1 exit status
    
    The problem is that in commit 7b01c1cc1d11 the fprintf_styled function
    was added into sis.c.  This file is only used when building the 'run'
    binary, that is, the standalone simulator, and is not included in the
    libsim.a library.
    
    Now, the obvious fix would be to move fprintf_styled into libsim.a,
    however, that turns out to be tricky.
    
    The erc32 simulator currently has two copies of the function run_sim,
    one in sis.c, and one in interf.c, both of these copies are global.
    
    Currently, the 'run' binary links fine, though I suspect this might be
    pure luck.  When I tried moving fprintf_styled into interf.c, I ran
    into multiple-definition (of run_sim) errors.  I suspect that by
    requiring the linker to pull in fprintf_styled from libsim.a I was
    changing the order in which symbols were loaded, and the linker was
    now seeing both copies of run_sim, while currently we only see one
    copy.
    
    The ideal solution of course, would be to merge the two similar, but
    slightly different copies of run_sim, and just use the one copy.  Then
    we could safely move fprintf_styled into interf.c too, and all would
    be good.
    
    But I don't have time right now to start debugging the erc32
    simulator, so I wanted a solution that fixes the build without
    introducing multiple definition errors.
    
    The easiest solution I think is to just have two copies of
    fprintf_styled, one in sis.c, and one in interf.c.  Unlike run_sim,
    these two copies are both static, so we will not run into multiple
    definition issues with this function.  The functions themselves are
    not very big, so it's not a huge amount of duplicate code.
    
    I am very aware that this is not an ideal solution, and I would
    welcome anyone who wants to take on fixing the run_sim problem
    properly, and then cleanup the fprintf_styled duplication.

diff --git a/sim/erc32/interf.c b/sim/erc32/interf.c
index 78dec6f4b9b..f433b9d55ac 100644
--- a/sim/erc32/interf.c
+++ b/sim/erc32/interf.c
@@ -156,6 +156,21 @@ run_sim(struct pstate *sregs, uint64_t icount, int dis)
     return TIME_OUT;
 }
 
+static int
+fprintf_styled (void *stream, enum disassembler_style style,
+		const char *fmt, ...)
+{
+  int ret;
+  FILE *out = (FILE *) stream;
+  va_list args;
+
+  va_start (args, fmt);
+  ret = vfprintf (out, fmt, args);
+  va_end (args);
+
+  return ret;
+}
+
 SIM_DESC
 sim_open (SIM_OPEN_KIND kind, struct host_callback_struct *callback,
 	  struct bfd *abfd, char * const *argv)
diff --git a/sim/erc32/sis.c b/sim/erc32/sis.c
index 12eb21f15a7..1d3ea139c23 100644
--- a/sim/erc32/sis.c
+++ b/sim/erc32/sis.c
@@ -138,7 +138,7 @@ run_sim(struct pstate *sregs, uint64_t icount, int dis)
     return TIME_OUT;
 }
 
-int
+static int
 fprintf_styled (void *stream, enum disassembler_style style,
 		const char *fmt, ...)
 {
diff --git a/sim/erc32/sis.h b/sim/erc32/sis.h
index 3a276670402..71033137f2c 100644
--- a/sim/erc32/sis.h
+++ b/sim/erc32/sis.h
@@ -204,8 +204,6 @@ extern void	init_regs (struct pstate *sregs);
 /* interf.c */
 extern int	run_sim (struct pstate *sregs,
 			 uint64_t icount, int dis);
-extern int      fprintf_styled (void *stream, enum disassembler_style style,
-				const char *fmt, ...) ATTRIBUTE_PRINTF (3, 4);
 
 /* float.c */
 extern int	get_accex (void);