Hi Lianbo,
Thank you for the fix.
I'm afraid of merging this kind of patch changing the initial value in
gdb before we will make the new release soon, but as far as I tested
there was no issue found. so let's merge this, but..
On 2024/03/06 21:32, lijiang wrote:
> On Wed, Mar 6, 2024 at 6:30 PM Daisuke Hatayama (Fujitsu)
> <d.hatayama@xxxxxxxxxxx <mailto:d.hatayama@xxxxxxxxxxx>> wrote:
>
> Lianbo,
>
> Thank you for your work.
>
> > Some objects format may potentially support copy relocations, but
> > currently the maybe_copied is always initialized to 0 in the
> symbol().
> > And the type is 'mst_file_bss', not always the 'mst_bss' or
> 'mst_data'
> > in the lookup_minimal_symbol_linkage(). For example:
> >
> > (gdb) p *msymbol
> > $42 = {<general_symbol_info> = {m_name = 0x349812f
> "test_no_static", value = {ivalue = 8, block = 0x8,
> > bytes = 0x8 <error: Cannot access memory at address 0x8>,
> address = 8, common_block = 0x8, chain = 0x8}, language_specific = {
> > obstack = 0x0, demangled_name = 0x0}, m_language =
> language_auto, ada_mangled = 0, section = 20}, size = 4,
> > filename = 0x6db3440 "test_sanity.c", type = mst_file_bss,
> created_by_gdb = 0, target_flag_1 = 0, target_flag_2 = 0, has_size = 1,
> > maybe_copied = 0, name_set = 1, hash_next = 0x0,
> demangled_hash_next = 0x0}
>
> The current description lacks explanation of when this issue
> occurs. Please write that the issue occurs when the corresponding
> kernel is built with CONFIG_CALL_DEPTH_TRACKING=y.
>
>
> Thank you for the comment, Hatayama.
>
> I should describe more background on this issue in the patch log. The
> current issue can be easily reproduced with the following kernel commit:
>
> commit 80e4c1cd42fff110bfdae8fce7ac4f22465f9664 (HEAD)
> Author: Thomas Gleixner <tglx@xxxxxxxxxxxxx <mailto:tglx@xxxxxxxxxxxxx>>
> Date: Thu Sep 15 13:11:19 2022 +0200
>
> x86/retbleed: Add X86_FEATURE_CALL_DEPTH
>
> Intel SKL CPUs fall back to other predictors when the RSB
> underflows. The
> only microcode mitigation is IBRS which is insanely expensive. It comes
> with performance drops of up to 30% depending on the workload.
>
> A way less expensive, but nevertheless horrible mitigation is to
> track the
> call depth in software and overeagerly fill the RSB when returns
> underflow
> the software counter.
>
> Provide a configuration symbol and a CPU misfeature bit.
>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx
> <mailto:tglx@xxxxxxxxxxxxx>>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx
> <mailto:peterz@xxxxxxxxxxxxx>>
> Link:
> https://lore.kernel.org/r/20220915111147.056176424@xxxxxxxxxxxxx
> <https://lore.kernel.org/r/20220915111147.056176424@xxxxxxxxxxxxx>
>
> After reverting the above commit, the current issue may disappear. And
> originally I tried to find the clue how this kernel commit changes
> affected the gdb, I have not found the clue for the time being. But
> later I noticed that the gdb gets the correct offset address of a global
> variable 'test_no_static', which is an expected behavior from the gdb
> perspective because of copy relocations, probably some object files
> potentially support the copy relocations, just like this.
>
> It would also be good to describe the fact that the issue occurs at
> least on RHEL9 kernel.
>
>
> This is an upstream issue, I have reproduced it on the upstream kernel
> with the above kernel commit changes.
The information that the issue occurs on RHEL9 kernel is helpful to RHEL
users. Why don't you add something like this?
---
The issue occurs with Linux 6.2 and later or kernels that have kernel
commit 80e4c1cd42ff ("x86/retbleed: Add X86_FEATURE_CALL_DEPTH") and
configured with CONFIG_CALL_DEPTH_TRACKING=y, including RHEL9.3 and
later kernels.
---
>
>
> > This causes a problem that the 'p' command can not work well as
> > expected, and always gets an error:
> >
> > crash> mod -s test_sanity /home/test_sanity.ko
> > MODULE NAME BASE
> SIZE OBJECT FILE
> > ffffffffc1084040 test_sanity ffffffffc1082000
> 16384 /home/test_sanity.ko
> > crash> p test_no_static
> > p: gdb request failed: p test_no_static
> > crash>
> >
> > With the patch:
> > crash> mod -s test_sanity /home/test_sanity.ko
> > MODULE NAME BASE
> SIZE OBJECT FILE
> > ffffffffc1084040 test_sanity ffffffffc1082000
> 16384 /home/test_sanity.ko
> > crash> p test_no_static
> > test_no_static = $1 = 5
> > crash>
>
> It's correct that p command doesn't work as expected, but it doesn't
> always result in some error. This issue is failure of calculating
> relocated address of static symbols. If the calculated address happens
> to be the address where read can be successfull, it doesn't result in
> read error but outputs some bogus value.
>
>
> It's true, but the bogus value is not an expected result because of an
> incorrect address.
I guess he means that the phrase only "always gets an error" is not
true, because it can emit a bogus value. so isn't "emits an error or a
bogus value" ok in this case?
I've rebased the patch and modified the commit log as my comments above
and attached. Lianbo, is this OK?
Thanks,
Kazu
> That is why the maybe_copied flag is initialized to 1, as I mentioned
> above, some objfile may potentially support the copy relocations.
>
>
> Thanks.
> Lianbo
>
>
> To make this clear, I think it's better to set debug level 4 and to
> have p command output calculated virtual address as debug messages.
>
> For example:
>
> crash> sym -M | grep -E " test_no"
> ffffffffc0da7580 (B) test_no
> ffffffffc0da7584 (b) test_no_static
> crash> set debug 4
> debug: 4
> crash> p test_no
> p: per_cpu_symbol_search(test_no): NULL
> test_no = <readmem: ffffffffc0da7580, KVADDR, "gdb_readmem
> callback", 4, (ROE), 560d2d483400>
> <read_diskdump: addr: ffffffffc0da7580 paddr: 10b263580 cnt: 4>
> $3 = 5
> crash> p test_no_static
> p: per_cpu_symbol_search(test_no_static): NULL
> test_no_static = <readmem: ffffffffc0d9f004, KVADDR,
> "gdb_readmem callback", 4, (ROE), 560d2dc9b100>
> <read_diskdump: addr: ffffffffc0d9f004 paddr: 108bfc004 cnt: 4>
> $4 = -1869574000
>
>
>
> --
> Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
> To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
> https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
> Contribution Guidelines: https://github.com/crash-utility/crash/wikiFrom 15a5aa01ffe7b13c3d11ae610756983873b4cce1 Mon Sep 17 00:00:00 2001
From: Lianbo Jiang <lijiang@xxxxxxxxxx>
Date: Wed, 6 Mar 2024 14:31:27 +0800
Subject: [PATCH] gdb: fix "p" command to print a module variable correctly
Some objects format may potentially support copy relocations, but
currently the maybe_copied is always initialized to 0 in the symbol().
And the type is 'mst_file_bss', not always the 'mst_bss' or 'mst_data'
in the lookup_minimal_symbol_linkage(). For example:
(gdb) p *msymbol
$42 = {<general_symbol_info> = {m_name = 0x349812f "test_no_static", value = {ivalue = 8, block = 0x8,
bytes = 0x8 <error: Cannot access memory at address 0x8>, address = 8, common_block = 0x8, chain = 0x8}, language_specific = {
obstack = 0x0, demangled_name = 0x0}, m_language = language_auto, ada_mangled = 0, section = 20}, size = 4,
filename = 0x6db3440 "test_sanity.c", type = mst_file_bss, created_by_gdb = 0, target_flag_1 = 0, target_flag_2 = 0, has_size = 1,
maybe_copied = 0, name_set = 1, hash_next = 0x0, demangled_hash_next = 0x0}
This causes a problem that the 'p' command cannot work well as expected,
and emits an error or a bogus value:
crash> mod -s test_sanity /home/test_sanity.ko
MODULE NAME BASE SIZE OBJECT FILE
ffffffffc1084040 test_sanity ffffffffc1082000 16384 /home/test_sanity.ko
crash> p test_no_static
p: gdb request failed: p test_no_static
crash>
The issue occurs with Linux 6.2 and later or kernels that have kernel
commit 80e4c1cd42ff ("x86/retbleed: Add X86_FEATURE_CALL_DEPTH") and
configured with CONFIG_CALL_DEPTH_TRACKING=y, including RHEL9.3 and
later kernels.
With the patch:
crash> mod -s test_sanity /home/test_sanity.ko
MODULE NAME BASE SIZE OBJECT FILE
ffffffffc1084040 test_sanity ffffffffc1082000 16384 /home/test_sanity.ko
crash> p test_no_static
test_no_static = $1 = 5
crash>
Signed-off-by: Lianbo Jiang <lijiang@xxxxxxxxxx>
---
gdb-10.2.patch | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/gdb-10.2.patch b/gdb-10.2.patch
index 7416efe..444e11a 100644
--- a/gdb-10.2.patch
+++ b/gdb-10.2.patch
@@ -16094,3 +16094,27 @@ exit 0
+{
+ loongarch_elf_fpregset.collect_regset (NULL, regcache, regno, fpregset, sizeof (prfpregset_t));
+}
+--- gdb-10.2/gdb/minsyms.c.orig
++++ gdb-10.2/gdb/minsyms.c
+@@ -535,7 +535,9 @@ lookup_minimal_symbol_linkage (const char *name, struct objfile *objf)
+ {
+ if (strcmp (msymbol->linkage_name (), name) == 0
+ && (MSYMBOL_TYPE (msymbol) == mst_data
+- || MSYMBOL_TYPE (msymbol) == mst_bss))
++ || MSYMBOL_TYPE (msymbol) == mst_bss
++ || MSYMBOL_TYPE (msymbol) == mst_file_bss
++ || MSYMBOL_TYPE (msymbol) == mst_file_data))
+ return {msymbol, objfile};
+ }
+ }
+--- gdb-10.2/gdb/symtab.h.orig
++++ gdb-10.2/gdb/symtab.h
+@@ -1110,7 +1110,7 @@ struct symbol : public general_symbol_info, public allocate_on_obstack
+ is_objfile_owned (1),
+ is_argument (0),
+ is_inlined (0),
+- maybe_copied (0),
++ maybe_copied (1), /* The objfile potentially supports copy relocations. */
+ subclass (SYMBOL_NONE)
+ {
+ /* We can't use an initializer list for members of a base class, and
--
2.39.3
--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
