Skip to content
This repository has been archived by the owner on Aug 17, 2022. It is now read-only.

RISC-V: Relax RISCV_PCREL_* to RISCV_GPREL_* #68

Merged
merged 2 commits into from
May 19, 2017

Conversation

palmer-dabbelt
Copy link
Contributor

In the medany code model the compiler generates PCREL_HI20+PCREL_LO12
relocation pairs against local symbols because HI20+LO12 relocations
can't reach high addresses. We relax HI20+LO12 pairs to GPREL
relocations when possible, which is an important optimization for
Dhrystone. Without this commit we are unable to relax
PCREL_HI20+PCREL_LO12 pairs to GPREL when possible, causing a XX%
permormance hit on Dhrystone on Rocket.

Note that we'll now relax

la gp, __global_pointer$

to

mv gp, gp

which probably isn't what you want in your entry code. Users who want
gp-relative symbols to continue to resolve should add ".option norelax"
accordingly. Due to this, the assembler now pairs PCREL relocations
with RELAX relocations when they're expected to be relaxed.

Copy link
Contributor

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Testing against Linux kernel now.

riscv_pcgp_lo_reloc *lo;
} riscv_pcgp_relocs;

static bfd_boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not void?

@palmer-dabbelt
Copy link
Contributor Author

This is broken, don't merge

Copy link
Contributor

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linux dun broke

@palmer-dabbelt palmer-dabbelt force-pushed the relax-pcrel-to-gprel-2 branch from 4e81b97 to b977766 Compare March 31, 2017 21:51
@palmer-dabbelt palmer-dabbelt force-pushed the riscv-binutils-2.28 branch 2 times, most recently from d58f11c to 77ad1e9 Compare May 1, 2017 20:26
@palmer-dabbelt palmer-dabbelt force-pushed the relax-pcrel-to-gprel-2 branch from b977766 to 4e29696 Compare May 2, 2017 23:21
@palmer-dabbelt palmer-dabbelt force-pushed the riscv-binutils-2.28 branch 4 times, most recently from 71e6586 to 3f21b5c Compare May 5, 2017 18:36
@palmer-dabbelt palmer-dabbelt force-pushed the relax-pcrel-to-gprel-2 branch 2 times, most recently from b1e2df9 to ce1d85a Compare May 16, 2017 00:52
@palmer-dabbelt
Copy link
Contributor Author

There's one remaining GCC test suite failure

gcc.dg/tree-prof/section-attr-1.c execution

@palmer-dabbelt
Copy link
Contributor Author

@aswaterman This now boots Linux and passes the GCC test suite. I've added R_RISCV_DELETE, which marks bytes for later deletion. We should be able to use this to make the relaxations linear time, but I haven't actually done that yet.

@aswaterman
Copy link
Contributor

R_RISCV_DELETE will never be emitted into an object file, right? If so, we should do something like #define R_RISCV_DELETE (R_RISCV_max + 1) in elfnn-riscv.c, rather than exposing it to the ELF ABI. I believe this also means we don't need BFD_RELOC_RISCV_DELETE.

@aswaterman aswaterman dismissed their stale review May 19, 2017 00:53

linux now boots

@palmer-dabbelt
Copy link
Contributor Author

Yes, that's better

@aswaterman
Copy link
Contributor

I'm OK with merging this locally, but my intuition is that we should prototype the linear-time relaxation pass before upstreaming this. I suspect that will reveal additional cobwebs.

@palmer-dabbelt
Copy link
Contributor Author

Ya, that was next on my TODO list. I'm going to clean this up and merge it for now, as I'm sick of needing to build binaries for people all the time.

@@ -89,6 +89,9 @@ START_RELOC_NUMBERS (elf_riscv_reloc_type)
RELOC_NUMBER (R_RISCV_SET32, 56)
END_RELOC_NUMBERS (R_RISCV_max)

/* Relocation types not emitted to binaries. */
#define R_RISCV_DELETE 127

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move to elfxx-riscv.h?

Did you pick 127 because of its ASCII meaning? :)

We currently delete bytes by shifting an entire BFD backwards to
overwrite the bytes we no longer need.  The result is that relaxing a
BFD is quadratic time.

This patch adds an additional relocation that specifies a byte range
that will be deleted from the final object file, and adds a relaxation
pass (between the existing passes that delete bytes and the alignment
pass) that actually deletes the bytes.  Note that deletion is still
quadratic time, and nothing uses R_RISCV_DELETE yet.

R_RISCV_DELETE will never be emitted into ELF objects, so therefor isn't exposed
to the rest of binutils.
In the medany code model the compiler generates PCREL_HI20+PCREL_LO12
relocation pairs against local symbols because HI20+LO12 relocations
can't reach high addresses.  We relax HI20+LO12 pairs to GPREL
relocations when possible, which is an important optimization for
Dhrystone.  Without this commit we are unable to relax
PCREL_HI20+PCREL_LO12 pairs to GPREL when possible, causing a XX%
permormance hit on Dhrystone on Rocket.

Note that we'll now relax

  la gp, __global_pointer$

to

  mv gp, gp

which probably isn't what you want in your entry code.  Users who want
gp-relative symbols to continue to resolve should add ".option norelax"
accordingly.  Due to this, the assembler now pairs PCREL relocations
with RELAX relocations when they're expected to be relaxed.
@palmer-dabbelt palmer-dabbelt force-pushed the relax-pcrel-to-gprel-2 branch from 70723ec to 7fe9c32 Compare May 19, 2017 01:15
@palmer-dabbelt palmer-dabbelt self-assigned this May 19, 2017
@palmer-dabbelt
Copy link
Contributor Author

I'm going to merge this, assuming it still passes the test suite.

@aswaterman
Copy link
Contributor

cool

@palmer-dabbelt palmer-dabbelt merged commit 3f69619 into riscv-binutils-2.28 May 19, 2017
@palmer-dabbelt palmer-dabbelt deleted the relax-pcrel-to-gprel-2 branch May 19, 2017 22:45
Nelson1225 pushed a commit to Nelson1225/riscv-binutils-gdb that referenced this pull request Nov 24, 2020
When loading the code file provided in PR 26828 and GDB is build with
UBSan, we get:

    Core was generated by `./Foo'.
    Program terminated with signal SIGABRT, Aborted.
    #0  0xb6c3809c in pthread_cond_wait () from /home/simark/build/binutils-gdb/gdb/repo/lib/libpthread.so.0
    [Current thread is 1 (LWP 29367)]
    (gdb) bt
    /home/simark/src/binutils-gdb/gdb/arm-tdep.c:1551:30: runtime error: shift exponent 32 is too large for 32-bit type 'unsigned int'

The sequence of instructions at pthread_cond_wait, in the
libpthread.so.0 library, contains this instruction with an immediate
constant with a "rotate amount" of 0:

    e24dd044        sub     sp, sp, riscvarchive#68     ; 0x44

Since arm_analyze_prologue shifts by "32 - rotate amount", it does a 32
bit shift of a 32 bit type, which is caught by UBSan.

Fix it by factoring out the decoding of immediates in a new function,
arm_expand_immediate.

I added a selftest for arm_analyze_prologue that replicates the
instruction sequence.  Without the fix, it crashes GDB if it is build
with --enable-ubsan.

I initially wanted to re-use the abstract_memory_reader class already in
arm-tdep.c, used to make arm_process_record testable.  However,
arm_process_record and arm_analyze_prologue don't use the same kind of
memory reading functions.  arm_process_record uses a function that
returns an error status on failure while arm_analyze_prologue uses one
that throws an exception.  Since i didn't want to introduce any other
behavior change, I decided to just introduce a separate interface
(arm_instruction_reader).  It is derived from
abstract_instruction_reader in aarch64-tdep.c.

gdb/ChangeLog:

	PR gdb/26835
	* arm-tdep.c (class arm_instruction_reader): New.
	(target_arm_instruction_reader): New.
	(arm_analyze_prologue): Add instruction reader parameter and use
	it.  Use arm_expand_immediate.
	(class target_arm_instruction_reader): Adjust.
	(arm_skip_prologue): Adjust.
	(arm_expand_immediate): New.
	(arm_scan_prologue): Adjust.
	(arm_analyze_prologue_test): New.
	(class test_arm_instruction_reader): New.

Change-Id: Ieb1c1799bd66f8c7421384f44f5c2777b578ff8d
Nelson1225 pushed a commit to Nelson1225/riscv-binutils-gdb that referenced this pull request Jul 22, 2021
As documented in bug 28086, test gdb.btrace/enable-new-thread.exp
started failing with commit 0618ae4 ("gdb: optimize
all_matching_threads_iterator"):

    (gdb) record btrace^M
    (gdb) PASS: gdb.btrace/enable-new-thread.exp: record btrace
    break 24^M
    Breakpoint 2 at 0x555555555175: file /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.btrace/enable-new-thread.c, line 24.^M
    (gdb) continue^M
    Continuing.^M
    /home/smarchi/src/binutils-gdb/gdb/inferior.c:303: internal-error: inferior* find_inferior_pid(process_stratum_target*, int): Assertion `pid != 0' failed.^M
    A problem internal to GDB has been detected,^M
    further debugging may prove unreliable.^M
    Quit this debugging session? (y or n) FAIL: gdb.btrace/enable-new-thread.exp: continue to breakpoint: cont to bp.1 (GDB internal error)

Note that I only see the failure if GDB is compiled without libipt
support.  This is because GDB then makes use BTS instead of PT, so
exercises different code paths.

I think that the commit above just exposed an existing problem.  The
stack trace of the internal error is:

    riscvarchive#8  0x0000561cb81e404e in internal_error (file=0x561cb83aa2f8 "/home/smarchi/src/binutils-gdb/gdb/inferior.c", line=303, fmt=0x561cb83aa099 "%s: Assertion `%s' failed.") at /home/smarchi/src/binutils-gdb/gdbsupport/errors.cc:55
    riscvarchive#9  0x0000561cb7b5c031 in find_inferior_pid (targ=0x561cb8aafb60 <the_amd64_linux_nat_target>, pid=0) at /home/smarchi/src/binutils-gdb/gdb/inferior.c:303
    riscvarchive#10 0x0000561cb7b5c102 in find_inferior_ptid (targ=0x561cb8aafb60 <the_amd64_linux_nat_target>, ptid=...) at /home/smarchi/src/binutils-gdb/gdb/inferior.c:317
    riscvarchive#11 0x0000561cb7f1d1c3 in find_thread_ptid (targ=0x561cb8aafb60 <the_amd64_linux_nat_target>, ptid=...) at /home/smarchi/src/binutils-gdb/gdb/thread.c:487
    riscvarchive#12 0x0000561cb7f1b921 in all_matching_threads_iterator::all_matching_threads_iterator (this=0x7ffc4ee34678, filter_target=0x561cb8aafb60 <the_amd64_linux_nat_target>, filter_ptid=...) at /home/smarchi/src/binutils-gdb/gdb/thread-iter.c:125
    riscvarchive#13 0x0000561cb77bc462 in filtered_iterator<all_matching_threads_iterator, non_exited_thread_filter>::filtered_iterator<process_stratum_target* const&, ptid_t const&> (this=0x7ffc4ee34670) at /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/filtered-iterator.h:42
    riscvarchive#14 0x0000561cb77b97cb in all_non_exited_threads_range::begin (this=0x7ffc4ee34650) at /home/smarchi/src/binutils-gdb/gdb/thread-iter.h:243
    riscvarchive#15 0x0000561cb7d8ba30 in record_btrace_target::record_is_replaying (this=0x561cb8aa6250 <record_btrace_ops>, ptid=...) at /home/smarchi/src/binutils-gdb/gdb/record-btrace.c:1411
    riscvarchive#16 0x0000561cb7d8bb83 in record_btrace_target::xfer_partial (this=0x561cb8aa6250 <record_btrace_ops>, object=TARGET_OBJECT_MEMORY, annex=0x0, readbuf=0x7ffc4ee34c58 "\260g\343N\374\177", writebuf=0x0, offset=140737352774277, len=1, xfered_len=0x7ffc4ee34ad8) at /home/smarchi/src/binutils-gdb/gdb/record-btrace.c:1437
    riscvarchive#17 0x0000561cb7ef73a9 in raw_memory_xfer_partial (ops=0x561cb8aa6250 <record_btrace_ops>, readbuf=0x7ffc4ee34c58 "\260g\343N\374\177", writebuf=0x0, memaddr=140737352774277, len=1, xfered_len=0x7ffc4ee34ad8) at /home/smarchi/src/binutils-gdb/gdb/target.c:1504
    riscvarchive#18 0x0000561cb7ef77da in memory_xfer_partial_1 (ops=0x561cb8aa6250 <record_btrace_ops>, object=TARGET_OBJECT_CODE_MEMORY, readbuf=0x7ffc4ee34c58 "\260g\343N\374\177", writebuf=0x0, memaddr=140737352774277, len=1, xfered_len=0x7ffc4ee34ad8) at /home/smarchi/src/binutils-gdb/gdb/target.c:1635
    riscvarchive#19 0x0000561cb7ef78b5 in memory_xfer_partial (ops=0x561cb8aa6250 <record_btrace_ops>, object=TARGET_OBJECT_CODE_MEMORY, readbuf=0x7ffc4ee34c58 "\260g\343N\374\177", writebuf=0x0, memaddr=140737352774277, len=1, xfered_len=0x7ffc4ee34ad8) at /home/smarchi/src/binutils-gdb/gdb/target.c:1664
    riscvarchive#20 0x0000561cb7ef7ba4 in target_xfer_partial (ops=0x561cb8aa6250 <record_btrace_ops>, object=TARGET_OBJECT_CODE_MEMORY, annex=0x0, readbuf=0x7ffc4ee34c58 "\260g\343N\374\177", writebuf=0x0, offset=140737352774277, len=1, xfered_len=0x7ffc4ee34ad8) at /home/smarchi/src/binutils-gdb/gdb/target.c:1721
    riscvarchive#21 0x0000561cb7ef8503 in target_read_partial (ops=0x561cb8aa6250 <record_btrace_ops>, object=TARGET_OBJECT_CODE_MEMORY, annex=0x0, buf=0x7ffc4ee34c58 "\260g\343N\374\177", offset=140737352774277, len=1, xfered_len=0x7ffc4ee34ad8) at /home/smarchi/src/binutils-gdb/gdb/target.c:1974
    riscvarchive#22 0x0000561cb7ef861f in target_read (ops=0x561cb8aa6250 <record_btrace_ops>, object=TARGET_OBJECT_CODE_MEMORY, annex=0x0, buf=0x7ffc4ee34c58 "\260g\343N\374\177", offset=140737352774277, len=1) at /home/smarchi/src/binutils-gdb/gdb/target.c:2014
    riscvarchive#23 0x0000561cb7ef809f in target_read_code (memaddr=140737352774277, myaddr=0x7ffc4ee34c58 "\260g\343N\374\177", len=1) at /home/smarchi/src/binutils-gdb/gdb/target.c:1869
    riscvarchive#24 0x0000561cb7937f4d in gdb_disassembler::dis_asm_read_memory (memaddr=140737352774277, myaddr=0x7ffc4ee34c58 "\260g\343N\374\177", len=1, info=0x7ffc4ee34e88) at /home/smarchi/src/binutils-gdb/gdb/disasm.c:139
    riscvarchive#25 0x0000561cb80ab66d in fetch_data (info=0x7ffc4ee34e88, addr=0x7ffc4ee34c59 "g\343N\374\177") at /home/smarchi/src/binutils-gdb/opcodes/i386-dis.c:194
    riscvarchive#26 0x0000561cb80ab7e2 in ckprefix () at /home/smarchi/src/binutils-gdb/opcodes/i386-dis.c:8628
    riscvarchive#27 0x0000561cb80adbd8 in print_insn (pc=140737352774277, info=0x7ffc4ee34e88) at /home/smarchi/src/binutils-gdb/opcodes/i386-dis.c:9587
    riscvarchive#28 0x0000561cb80abe4f in print_insn_i386 (pc=140737352774277, info=0x7ffc4ee34e88) at /home/smarchi/src/binutils-gdb/opcodes/i386-dis.c:8894
    riscvarchive#29 0x0000561cb7744a19 in default_print_insn (memaddr=140737352774277, info=0x7ffc4ee34e88) at /home/smarchi/src/binutils-gdb/gdb/arch-utils.c:1029
    riscvarchive#30 0x0000561cb7b33067 in i386_print_insn (pc=140737352774277, info=0x7ffc4ee34e88) at /home/smarchi/src/binutils-gdb/gdb/i386-tdep.c:4013
    riscvarchive#31 0x0000561cb7acd8f4 in gdbarch_print_insn (gdbarch=0x561cbae2fb60, vma=140737352774277, info=0x7ffc4ee34e88) at /home/smarchi/src/binutils-gdb/gdb/gdbarch.c:3478
    riscvarchive#32 0x0000561cb793a32d in gdb_disassembler::print_insn (this=0x7ffc4ee34e80, memaddr=140737352774277, branch_delay_insns=0x0) at /home/smarchi/src/binutils-gdb/gdb/disasm.c:795
    riscvarchive#33 0x0000561cb793a5b0 in gdb_print_insn (gdbarch=0x561cbae2fb60, memaddr=140737352774277, stream=0x561cb8ac99f8 <null_stream>, branch_delay_insns=0x0) at /home/smarchi/src/binutils-gdb/gdb/disasm.c:850
    riscvarchive#34 0x0000561cb793a631 in gdb_insn_length (gdbarch=0x561cbae2fb60, addr=140737352774277) at /home/smarchi/src/binutils-gdb/gdb/disasm.c:859
    riscvarchive#35 0x0000561cb77f53f4 in btrace_compute_ftrace_bts (tp=0x561cbba11210, btrace=0x7ffc4ee35188, gaps=...) at /home/smarchi/src/binutils-gdb/gdb/btrace.c:1107
    riscvarchive#36 0x0000561cb77f55f5 in btrace_compute_ftrace_1 (tp=0x561cbba11210, btrace=0x7ffc4ee35180, cpu=0x0, gaps=...) at /home/smarchi/src/binutils-gdb/gdb/btrace.c:1527
    riscvarchive#37 0x0000561cb77f5705 in btrace_compute_ftrace (tp=0x561cbba11210, btrace=0x7ffc4ee35180, cpu=0x0) at /home/smarchi/src/binutils-gdb/gdb/btrace.c:1560
    riscvarchive#38 0x0000561cb77f583b in btrace_add_pc (tp=0x561cbba11210) at /home/smarchi/src/binutils-gdb/gdb/btrace.c:1589
    riscvarchive#39 0x0000561cb77f5a86 in btrace_enable (tp=0x561cbba11210, conf=0x561cb8ac6878 <record_btrace_conf>) at /home/smarchi/src/binutils-gdb/gdb/btrace.c:1629
    riscvarchive#40 0x0000561cb7d88d26 in record_btrace_enable_warn (tp=0x561cbba11210) at /home/smarchi/src/binutils-gdb/gdb/record-btrace.c:294
    riscvarchive#41 0x0000561cb7c603dc in std::__invoke_impl<void, void (*&)(thread_info*), thread_info*> (__f=@0x561cbb6c4878: 0x561cb7d88cdc <record_btrace_enable_warn(thread_info*)>) at /usr/include/c++/10/bits/invoke.h:60
    riscvarchive#42 0x0000561cb7c5e5a6 in std::__invoke_r<void, void (*&)(thread_info*), thread_info*> (__fn=@0x561cbb6c4878: 0x561cb7d88cdc <record_btrace_enable_warn(thread_info*)>) at /usr/include/c++/10/bits/invoke.h:153
    riscvarchive#43 0x0000561cb7c5dc92 in std::_Function_handler<void (thread_info*), void (*)(thread_info*)>::_M_invoke(std::_Any_data const&, thread_info*&&) (__functor=..., __args#0=@0x7ffc4ee35310: 0x561cbba11210) at /usr/include/c++/10/bits/std_function.h:291
    riscvarchive#44 0x0000561cb7f2600f in std::function<void (thread_info*)>::operator()(thread_info*) const (this=0x561cbb6c4878, __args#0=0x561cbba11210) at /usr/include/c++/10/bits/std_function.h:622
    riscvarchive#45 0x0000561cb7f23dc8 in gdb::observers::observable<thread_info*>::notify (this=0x561cb8ac5aa0 <gdb::observers::new_thread>, args#0=0x561cbba11210) at /home/smarchi/src/binutils-gdb/gdb/../gdbsupport/observable.h:150
    riscvarchive#46 0x0000561cb7f1c436 in add_thread_silent (targ=0x561cb8aafb60 <the_amd64_linux_nat_target>, ptid=...) at /home/smarchi/src/binutils-gdb/gdb/thread.c:263
    riscvarchive#47 0x0000561cb7f1c479 in add_thread_with_info (targ=0x561cb8aafb60 <the_amd64_linux_nat_target>, ptid=..., priv=0x561cbb3f7ab0) at /home/smarchi/src/binutils-gdb/gdb/thread.c:272
    riscvarchive#48 0x0000561cb7bfa1d0 in record_thread (info=0x561cbb0413a0, tp=0x0, ptid=..., th_p=0x7ffc4ee35610, ti_p=0x7ffc4ee35620) at /home/smarchi/src/binutils-gdb/gdb/linux-thread-db.c:1380
    riscvarchive#49 0x0000561cb7bf7a2a in thread_from_lwp (stopped=0x561cba81db20, ptid=...) at /home/smarchi/src/binutils-gdb/gdb/linux-thread-db.c:429
    riscvarchive#50 0x0000561cb7bf7ac5 in thread_db_notice_clone (parent=..., child=...) at /home/smarchi/src/binutils-gdb/gdb/linux-thread-db.c:447
    riscvarchive#51 0x0000561cb7bdc9a2 in linux_handle_extended_wait (lp=0x561cbae25720, status=4991) at /home/smarchi/src/binutils-gdb/gdb/linux-nat.c:1981
    riscvarchive#52 0x0000561cb7bdf0f3 in linux_nat_filter_event (lwpid=435403, status=198015) at /home/smarchi/src/binutils-gdb/gdb/linux-nat.c:2920
    riscvarchive#53 0x0000561cb7bdfed6 in linux_nat_wait_1 (ptid=..., ourstatus=0x7ffc4ee36398, target_options=...) at /home/smarchi/src/binutils-gdb/gdb/linux-nat.c:3202
    riscvarchive#54 0x0000561cb7be0b68 in linux_nat_target::wait (this=0x561cb8aafb60 <the_amd64_linux_nat_target>, ptid=..., ourstatus=0x7ffc4ee36398, target_options=...) at /home/smarchi/src/binutils-gdb/gdb/linux-nat.c:3440
    riscvarchive#55 0x0000561cb7bfa2fc in thread_db_target::wait (this=0x561cb8a9acd0 <the_thread_db_target>, ptid=..., ourstatus=0x7ffc4ee36398, options=...) at /home/smarchi/src/binutils-gdb/gdb/linux-thread-db.c:1412
    riscvarchive#56 0x0000561cb7d8e356 in record_btrace_target::wait (this=0x561cb8aa6250 <record_btrace_ops>, ptid=..., status=0x7ffc4ee36398, options=...) at /home/smarchi/src/binutils-gdb/gdb/record-btrace.c:2547
    riscvarchive#57 0x0000561cb7ef996d in target_wait (ptid=..., status=0x7ffc4ee36398, options=...) at /home/smarchi/src/binutils-gdb/gdb/target.c:2608
    riscvarchive#58 0x0000561cb7b6d297 in do_target_wait_1 (inf=0x561cba6d8780, ptid=..., status=0x7ffc4ee36398, options=...) at /home/smarchi/src/binutils-gdb/gdb/infrun.c:3640
    riscvarchive#59 0x0000561cb7b6d43e in operator() (__closure=0x7ffc4ee36190, inf=0x561cba6d8780) at /home/smarchi/src/binutils-gdb/gdb/infrun.c:3701
    riscvarchive#60 0x0000561cb7b6d7b2 in do_target_wait (ecs=0x7ffc4ee36370, options=...) at /home/smarchi/src/binutils-gdb/gdb/infrun.c:3720
    riscvarchive#61 0x0000561cb7b6e67d in fetch_inferior_event () at /home/smarchi/src/binutils-gdb/gdb/infrun.c:4069
    riscvarchive#62 0x0000561cb7b4659b in inferior_event_handler (event_type=INF_REG_EVENT) at /home/smarchi/src/binutils-gdb/gdb/inf-loop.c:41
    riscvarchive#63 0x0000561cb7be25f7 in handle_target_event (error=0, client_data=0x0) at /home/smarchi/src/binutils-gdb/gdb/linux-nat.c:4227
    riscvarchive#64 0x0000561cb81e4ee2 in handle_file_event (file_ptr=0x561cbae24e10, ready_mask=1) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:575
    riscvarchive#65 0x0000561cb81e5490 in gdb_wait_for_event (block=0) at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:701
    riscvarchive#66 0x0000561cb81e41be in gdb_do_one_event () at /home/smarchi/src/binutils-gdb/gdbsupport/event-loop.cc:212
    riscvarchive#67 0x0000561cb7c18096 in start_event_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:421
    riscvarchive#68 0x0000561cb7c181e0 in captured_command_loop () at /home/smarchi/src/binutils-gdb/gdb/main.c:481
    riscvarchive#69 0x0000561cb7c19d7e in captured_main (data=0x7ffc4ee366a0) at /home/smarchi/src/binutils-gdb/gdb/main.c:1353
    riscvarchive#70 0x0000561cb7c19df0 in gdb_main (args=0x7ffc4ee366a0) at /home/smarchi/src/binutils-gdb/gdb/main.c:1368
    riscvarchive#71 0x0000561cb7693186 in main (argc=11, argv=0x7ffc4ee367b8) at /home/smarchi/src/binutils-gdb/gdb/gdb.c:32

At frame 45, the new_thread observable is fired.  At this moment, the
new thread isn't the current thread, inferior_ptid is null_ptid.  I
think this is ok: the new_thread observable doesn't give any guarantee
on the global context when observers are invoked.  Frame 35,
btrace_compute_ftrace_bts, calls gdb_insn_length.  gdb_insn_length
doesn't have a thread_info or other parameter what could indicate where
to read memory from, it implicitly uses the global context
(inferior_ptid).

So we reach the all_non_exited_threads_range in
record_btrace_target::record_is_replaying with a null inferior_ptid.
The previous implemention of all_non_exited_threads_range didn't care,
but the new one does.  The problem of calling gdb_insn_length and
ultimately trying to read memory with a null inferior_ptid already
existed, but the commit mentioned above made it visible.

Something between frames 40 (record_btrace_enable_warn) and 35
(btrace_compute_ftrace_bts) needs to be switching the global context to
make TP the current thread.  Since btrace_compute_ftrace_bts takes the
thread_info to work with as a parameter, that typically means that it
doesn't require its caller to also set the global current context
(current thread) when calling.  If it needs to call other functions
that do require the global current thread to be set, then it needs to
temporarily change the current thread while calling these other
functions.  Therefore, switch and restore the current thread in
btrace_compute_ftrace_bts.

By inspection, it looks like btrace_compute_ftrace_pt may also call
functions sensitive to the global context: it installs the
btrace_pt_readmem_callback callback in the PT instruction decoder.  When
this function gets called, inferior_ptid must be set appropriately.  Add
a switch and restore in there too.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28086
Change-Id: I407fbfe41aab990068bd102491aa3709b0a034b3
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants