* [PATCH/tilegx] tilegx bug fixes & improvements
@ 2012-12-06 7:00 Jiong Wang
2012-12-06 9:39 ` Yao Qi
0 siblings, 1 reply; 6+ messages in thread
From: Jiong Wang @ 2012-12-06 7:00 UTC (permalink / raw)
To: gdb-patches; +Cc: Walter Lee, palves
[-- Attachment #1: Type: text/plain, Size: 728 bytes --]
Hi All,
attachments fix several bugs on tilegx exposed by dejagnu and improve
tilegx gdb's info register output.
after these patches, the dejagnu test result improved from:
=== gdb Summary ===
# of expected passes 19030
# of unexpected failures 116
to:
=== gdb Summary ===
# of expected passes 19100
# of unexpected failures 61
these patches are against the latest commit:
commit 2cca06a0bf0f859e20276e5c614db7b325753a8d
Author: Joel Brobecker <brobecker@gnat.com>
Date: Thu Dec 6 04:57:06 2012 +0000
aix-thread: Fix getthrds declaration and call.
please review, thanks
--
Regards,
Jiong. Wang
Tilera Corporation
[-- Attachment #2: 0001-tilegx-tdep.c-tilegx_push_dummy_call-stack-alignment.patch --]
[-- Type: text/x-patch, Size: 2744 bytes --]
From 85696377fc2c76ea51463c5896f63c96a5ace92e Mon Sep 17 00:00:00 2001
From: Jiong Wang <jiwang@tilera.com>
Date: Thu, 6 Dec 2012 12:13:03 +0800
Subject: [PATCH 1/5] * tilegx-tdep.c (tilegx_push_dummy_call): stack
alignment should be 64bit
for tilegx, when push args on stack, the alignment should be 64bit
---
gdb/tilegx-tdep.c | 32 ++++++++------------------------
1 file changed, 8 insertions(+), 24 deletions(-)
diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index 627a470..2f7f253 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -292,7 +292,7 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
int argreg = TILEGX_R0_REGNUM;
int i, j;
int typelen, slacklen, alignlen;
- static const gdb_byte two_zero_words[8] = { 0 };
+ static const gdb_byte four_zero_words[16] = { 0 };
/* If struct_return is 1, then the struct return address will
consume one argument-passing register. */
@@ -324,44 +324,28 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
}
/* Align SP. */
- stack_dest = tilegx_frame_align (gdbarch, stack_dest);
-
- /* Loop backwards through arguments to determine stack alignment. */
- alignlen = 0;
-
- for (j = nargs - 1; j >= i; j--)
- {
- typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
- alignlen += (typelen + 3) & (~3);
- }
-
- if (alignlen & 0x4)
- stack_dest -= 4;
+ stack_dest = (stack_dest + 7) & ~0x7;
/* Loop backwards through remaining arguments and push them on
the stack, word aligned. */
for (j = nargs - 1; j >= i; j--)
{
gdb_byte *val;
- struct cleanup *back_to;
- const gdb_byte *contents = value_contents (args[j]);
typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
- slacklen = ((typelen + 3) & (~3)) - typelen;
- val = xmalloc (typelen + slacklen);
- back_to = make_cleanup (xfree, val);
- memcpy (val, contents, typelen);
+ slacklen = ((typelen + 7) & (~7)) - typelen;
+ val = alloca (typelen + slacklen);
+ memcpy (val, value_contents (args[j]), typelen);
memset (val + typelen, 0, slacklen);
/* Now write data to the stack. The stack grows downwards. */
stack_dest -= typelen + slacklen;
write_memory (stack_dest, val, typelen + slacklen);
- do_cleanups (back_to);
}
- /* Add 2 words for linkage space to the stack. */
- stack_dest = stack_dest - 8;
- write_memory (stack_dest, two_zero_words, 8);
+ /* Add 2 double words for linkage space to the stack. */
+ stack_dest = stack_dest - 16;
+ write_memory (stack_dest, four_zero_words, 16);
/* Update stack pointer. */
regcache_cooked_write_unsigned (regcache, TILEGX_SP_REGNUM, stack_dest);
--
1.7.10.4
[-- Attachment #3: 0002-tilegx-tdep.c-tilegx_analyze_prologue-fix-prologue-a.patch --]
[-- Type: text/x-patch, Size: 1867 bytes --]
From 028cafa74fe2601ebf13e9a64e6de90e891cbed8 Mon Sep 17 00:00:00 2001
From: Jiong Wang <jiwang@tilera.com>
Date: Thu, 6 Dec 2012 12:15:32 +0800
Subject: [PATCH 2/5] * tilegx-tdep.c (tilegx_analyze_prologue): fix
prologue analysis overflow bug
when setting breakpoint by function name, if that function
is in .so and that .so is not loaded yet, then gdb will do
partial name match, which will match the corresponding plt
stub entry. We should take this situation into account when
doing prologue analysis.
---
gdb/tilegx-tdep.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index 2f7f253..93969c9 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -432,8 +432,22 @@ tilegx_analyze_prologue (struct gdbarch* gdbarch,
status = safe_frame_unwind_memory (next_frame, instbuf_start,
instbuf, instbuf_size);
- if (status == 0)
- memory_error (status, next_addr);
+ if (status == 0) {
+ /* fix gdb.base/gdb1250
+ * breakpoint is set before dynamic library loaded, thus gdb
+ * does a partial symbol name finding and sets the breakpoint
+ * in the plt stub. our 32-bundle prefetch window is too large
+ * for this situation to cause a memory access error.
+ * For plt stub, we just need to return directly.
+ *
+ * x86 does not have this problem, because the first instruction
+ * in their plt stub is jump, which ends the analysis also.
+ */
+ if (strcmp(find_pc_section(instbuf_start)->the_bfd_section->name,
+ ".plt") == 0)
+ return instbuf_start;
+ memory_error (status, next_addr);
+ }
}
reverse_frame_valid = 0;
--
1.7.10.4
[-- Attachment #4: 0003-tilegx-tdep.c-tilegx_write_pc-prevent-kernel-from-re.patch --]
[-- Type: text/x-patch, Size: 5072 bytes --]
From 2ecf3fa49f1c2e5031762e0558b1f0c8f37103b4 Mon Sep 17 00:00:00 2001
From: Jiong Wang <jiwang@tilera.com>
Date: Thu, 6 Dec 2012 12:16:15 +0800
Subject: [PATCH 3/5] * tilegx-tdep.c (tilegx_write_pc): prevent kernel
from restarting syscall for gdb hand call
if we just interrupted a system call, the kernel might
try to restart it when we resume the inferior, such as
nanosleep.
for gdb, we may resume the inferior because of a hand call.
in this situation, we should prevent kernel from restarting
syscall at $pc - 8, which is no longer the original place
where syscall occured.
---
gdb/tilegx-linux-nat.c | 2 +-
gdb/tilegx-linux-tdep.c | 6 ++++--
gdb/tilegx-tdep.c | 32 ++++++++++++++++++++++++++++++--
gdb/tilegx-tdep.h | 4 ++--
4 files changed, 37 insertions(+), 7 deletions(-)
diff --git a/gdb/tilegx-linux-nat.c b/gdb/tilegx-linux-nat.c
index 23acba5..51dbf5e 100644
--- a/gdb/tilegx-linux-nat.c
+++ b/gdb/tilegx-linux-nat.c
@@ -65,7 +65,7 @@ static const int regmap[] =
40, 41, 42, 43, 44, 45, 46, 47,
48, 49, 50, 51, 52, 53, 54, 55,
-1, -1, -1, -1, -1, -1, -1, -1,
- 56
+ 56, 58
};
/* Transfering the general-purpose registers between GDB, inferiors
diff --git a/gdb/tilegx-linux-tdep.c b/gdb/tilegx-linux-tdep.c
index c5e0e80..19ad297 100644
--- a/gdb/tilegx-linux-tdep.c
+++ b/gdb/tilegx-linux-tdep.c
@@ -85,9 +85,11 @@ tilegx_linux_supply_regset (const struct regset *regset,
int i;
/* This logic must match that of struct pt_regs in "ptrace.h". */
- for (i = 0; i < TILEGX_NUM_EASY_REGS + 1; i++, ptr += tilegx_reg_size)
+ for (i = 0; i < TILEGX_NUM_EASY_REGS + 2; i++, ptr += tilegx_reg_size)
{
- int gri = (i < TILEGX_NUM_EASY_REGS) ? i : TILEGX_PC_REGNUM;
+ int gri = (i < TILEGX_NUM_EASY_REGS)
+ ? i : (i == TILEGX_NUM_EASY_REGS)
+ ? TILEGX_PC_REGNUM : TILEGX_FAULTNUM_REGNUM;
if (regnum == gri || regnum == -1)
regcache_raw_supply (regcache, gri, ptr);
diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index 93969c9..bc8cd35 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -155,7 +155,7 @@ tilegx_register_name (struct gdbarch *gdbarch, int regnum)
"r40", "r41", "r42", "r43", "r44", "r45", "r46", "r47",
"r48", "r49", "r50", "r51", "r52", "tp", "sp", "lr",
"sn", "idn0", "idn1", "udn0", "udn1", "udn2", "udn3", "zero",
- "pc"
+ "pc", "faultnum",
};
if (regnum < 0 || regnum >= TILEGX_NUM_REGS)
@@ -782,6 +782,32 @@ tilegx_in_function_epilogue_p (struct gdbarch *gdbarch, CORE_ADDR pc)
return 0;
}
+#define INT_SWINT_1_SIGRETURN (~0)
+static void
+tilegx_write_pc (struct regcache *regcache, CORE_ADDR pc)
+{
+ regcache_cooked_write_unsigned (regcache, TILEGX_PC_REGNUM, pc);
+
+ /* We must be careful with modifying the program counter. If we
+ just interrupted a system call, the kernel might try to restart
+ it when we resume the inferior. On restarting the system call,
+ the kernel will try backing up the program counter even though it
+ no longer points at the system call. This typically results in a
+ SIGSEGV or SIGILL. We can prevent this by writing INT_SWINT_1_SIGRETURN
+ in the "faultnum" pseudo-register.
+
+ Note that "faultnum" is saved when setting up a dummy call frame.
+ This means that it is properly restored when that frame is
+ popped, and that the interrupted system call will be restarted
+ when we resume the inferior on return from a function call from
+ within GDB. In all other cases the system call will not be
+ restarted. */
+ regcache_cooked_write_unsigned (regcache, TILEGX_FAULTNUM_REGNUM,
+ INT_SWINT_1_SIGRETURN);
+}
+
+
+
/* This is the implementation of gdbarch method breakpoint_from_pc. */
static const unsigned char *
@@ -913,7 +939,8 @@ tilegx_cannot_reference_register (struct gdbarch *gdbarch, int regno)
{
if (regno >= 0 && regno < TILEGX_NUM_EASY_REGS)
return 0;
- else if (regno == TILEGX_PC_REGNUM)
+ else if (regno == TILEGX_PC_REGNUM
+ || regno == TILEGX_FAULTNUM_REGNUM)
return 0;
else
return 1;
@@ -996,6 +1023,7 @@ tilegx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
/* These values and methods are used when gdb calls a target function. */
set_gdbarch_push_dummy_call (gdbarch, tilegx_push_dummy_call);
+ set_gdbarch_write_pc (gdbarch, tilegx_write_pc);
set_gdbarch_breakpoint_from_pc (gdbarch, tilegx_breakpoint_from_pc);
set_gdbarch_return_value (gdbarch, tilegx_return_value);
diff --git a/gdb/tilegx-tdep.h b/gdb/tilegx-tdep.h
index 3ac18a5..5f99fd9 100644
--- a/gdb/tilegx-tdep.h
+++ b/gdb/tilegx-tdep.h
@@ -100,8 +100,8 @@ enum tilegx_regnum
TILEGX_PC_REGNUM,
TILEGX_NUM_PHYS_REGS = TILEGX_PC_REGNUM, /* 64 */
-
- TILEGX_NUM_REGS /* 65 */
+ TILEGX_FAULTNUM_REGNUM,
+ TILEGX_NUM_REGS, /* 66 */
};
enum { tilegx_reg_size = 8 };
--
1.7.10.4
[-- Attachment #5: 0004-tilegx-tdep.c-tilegx_skip_prologue-use-skip_prologue.patch --]
[-- Type: text/x-patch, Size: 1733 bytes --]
From 42a5f9a8d1caffda706d1371f1cfa5b2490bef03 Mon Sep 17 00:00:00 2001
From: Jiong Wang <jiwang@tilera.com>
Date: Thu, 6 Dec 2012 14:11:08 +0800
Subject: [PATCH 4/5] * tilegx-tdep.c (tilegx_skip_prologue): use
skip_prologue_using_sal instead of find_pc_line
---
gdb/tilegx-tdep.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index bc8cd35..1e967fe 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -741,24 +741,24 @@ tilegx_analyze_prologue (struct gdbarch* gdbarch,
/* This is the implementation of gdbarch method skip_prologue. */
static CORE_ADDR
-tilegx_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
+tilegx_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
{
- struct symtab_and_line sal;
- CORE_ADDR func_start, func_end;
+ CORE_ADDR func_start;
/* This is the preferred method, find the end of the prologue by
using the debugging information. */
- if (find_pc_partial_function (pc, NULL, &func_start, &func_end))
+ if (find_pc_partial_function (start_pc, NULL, &func_start, NULL))
{
- sal = find_pc_line (func_start, 0);
-
- if (sal.end < func_end && pc <= sal.end)
- return sal.end;
+ CORE_ADDR post_prologue_pc
+ = skip_prologue_using_sal (gdbarch, func_start);
+ if (post_prologue_pc != 0)
+ return max (start_pc, post_prologue_pc);
}
/* Otherwise, try to skip prologue the hard way. */
return tilegx_analyze_prologue (gdbarch,
- pc, pc + 8 * TILEGX_BUNDLE_SIZE_IN_BYTES,
+ start_pc,
+ start_pc + 8 * TILEGX_BUNDLE_SIZE_IN_BYTES,
NULL, NULL);
}
--
1.7.10.4
[-- Attachment #6: 0005-tilegx-tdep.c-tilera_print_registers_info-new.patch --]
[-- Type: text/x-patch, Size: 2796 bytes --]
From d6ea11561793c96cf1eb9f3f013089724c9c923d Mon Sep 17 00:00:00 2001
From: Jiong Wang <jiwang@tilera.com>
Date: Thu, 6 Dec 2012 14:15:54 +0800
Subject: [PATCH 5/5] * tilegx-tdep.c (tilera_print_registers_info): new
support tilegx private register info hook to show
registers in columns.
---
gdb/tilegx-tdep.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 63 insertions(+)
diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index 1e967fe..bf00983 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -946,6 +946,66 @@ tilegx_cannot_reference_register (struct gdbarch *gdbarch, int regno)
return 1;
}
+
+/* Tilera private register printer. */
+#define MAX_COLUMNS 3
+static void
+tilera_print_registers_info (struct gdbarch *gdbarch, struct ui_file *file,
+ struct frame_info *frame,
+ int regnum, int print_all)
+{
+ int i;
+ int j;
+ int k;
+ gdb_byte buffer[MAX_REGISTER_SIZE];
+
+ if (regnum != -1)
+ {
+ default_print_registers_info (gdbarch, file, frame, regnum, print_all);
+ return;
+ }
+
+ for (i = 0; i < TILEGX_NUM_EASY_REGS / MAX_COLUMNS + 1; ++i)
+ {
+ for (j = i;
+ j < TILEGX_NUM_EASY_REGS + 1;
+ j += TILEGX_NUM_EASY_REGS / MAX_COLUMNS + 1)
+ {
+ /* Some registers are never available. Skip them and print PC. */
+ if (j > TILEGX_LR_REGNUM)
+ j = TILEGX_PC_REGNUM;
+
+ if (j > i)
+ fprintf_filtered (file, " ");
+
+ fputs_filtered (gdbarch_register_name (gdbarch, j), file);
+ print_spaces_filtered (5 - strlen (gdbarch_register_name
+ (gdbarch, j)), file);
+
+ /* Get the data in raw format. */
+ if (! deprecated_frame_register_read (frame, j, buffer))
+ {
+ fprintf_filtered (file, " *** no value *** ");
+ }
+ else
+ {
+ fprintf_filtered (file, "0x");
+ for (k = 0; k < tilegx_reg_size; k++)
+ {
+ int idx;
+ if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
+ idx = k;
+ else
+ idx = tilegx_reg_size - 1 - k;
+ fprintf_filtered (file, "%02x", (unsigned char) buffer[idx]);
+ }
+ }
+ }
+
+ fprintf_filtered (file, "\n");
+ }
+}
+
static struct gdbarch *
tilegx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
{
@@ -1006,6 +1066,9 @@ tilegx_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
/* Stack grows down. */
set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
+ /* Tilera private register printer */
+ set_gdbarch_print_registers_info (gdbarch, tilera_print_registers_info);
+
/* Frame Info. */
set_gdbarch_unwind_sp (gdbarch, tilegx_unwind_sp);
set_gdbarch_unwind_pc (gdbarch, tilegx_unwind_pc);
--
1.7.10.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/tilegx] tilegx bug fixes & improvements
2012-12-06 7:00 [PATCH/tilegx] tilegx bug fixes & improvements Jiong Wang
@ 2012-12-06 9:39 ` Yao Qi
2012-12-06 15:20 ` Jiong Wang
0 siblings, 1 reply; 6+ messages in thread
From: Yao Qi @ 2012-12-06 9:39 UTC (permalink / raw)
To: Jiong Wang; +Cc: gdb-patches, Walter Lee, palves
On 12/06/2012 02:59 PM, Jiong Wang wrote:
> Hi All,
>
> attachments fix several bugs on tilegx exposed by dejagnu and improve
> tilegx gdb's info register output.
>
> after these patches, the dejagnu test result improved from:
>
> === gdb Summary ===
>
> # of expected passes 19030
> # of unexpected failures 116
>
> to:
> === gdb Summary ===
>
> # of expected passes 19100
> # of unexpected failures 61
>
The result looks good.
We don't attach multiple patches into a single mail. Assuming each of
your patches is independent, you can e-mail them one by one. In each
mail, the ChangeLog entry and description is included, helping people to
understand the patch.
>
>
> 0001-tilegx-tdep.c-tilegx_push_dummy_call-stack-alignment.patch
>
>
> From 85696377fc2c76ea51463c5896f63c96a5ace92e Mon Sep 17 00:00:00 2001
> From: Jiong Wang<jiwang@tilera.com>
> Date: Thu, 6 Dec 2012 12:13:03 +0800
> Subject: [PATCH 1/5] * tilegx-tdep.c (tilegx_push_dummy_call): stack
> alignment should be 64bit
>
> for tilegx, when push args on stack, the alignment should be 64bit
> ---
> gdb/tilegx-tdep.c | 32 ++++++++------------------------
> 1 file changed, 8 insertions(+), 24 deletions(-)
>
> diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
> index 627a470..2f7f253 100644
> --- a/gdb/tilegx-tdep.c
> +++ b/gdb/tilegx-tdep.c
> @@ -292,7 +292,7 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
> int argreg = TILEGX_R0_REGNUM;
> int i, j;
> int typelen, slacklen, alignlen;
> - static const gdb_byte two_zero_words[8] = { 0 };
> + static const gdb_byte four_zero_words[16] = { 0 };
>
> /* If struct_return is 1, then the struct return address will
> consume one argument-passing register. */
> @@ -324,44 +324,28 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
> }
>
> /* Align SP. */
> - stack_dest = tilegx_frame_align (gdbarch, stack_dest);
> -
> - /* Loop backwards through arguments to determine stack alignment. */
> - alignlen = 0;
> -
> - for (j = nargs - 1; j >= i; j--)
> - {
> - typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
> - alignlen += (typelen + 3) & (~3);
> - }
> -
> - if (alignlen & 0x4)
> - stack_dest -= 4;
> + stack_dest = (stack_dest + 7) & ~0x7;
Your can use function 'align_up'.
>
> /* Loop backwards through remaining arguments and push them on
> the stack, word aligned. */
> for (j = nargs - 1; j >= i; j--)
> {x
> gdb_byte *val;
> - struct cleanup *back_to;
> - const gdb_byte *contents = value_contents (args[j]);
>
> typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
> - slacklen = ((typelen + 3) & (~3)) - typelen;
> - val = xmalloc (typelen + slacklen);
> - back_to = make_cleanup (xfree, val);
> - memcpy (val, contents, typelen);
> + slacklen = ((typelen + 7) & (~7)) - typelen;
> + val = alloca (typelen + slacklen);
> + memcpy (val, value_contents (args[j]), typelen);
> memset (val + typelen, 0, slacklen);
>
> /* Now write data to the stack. The stack grows downwards. */
> stack_dest -= typelen + slacklen;
> write_memory (stack_dest, val, typelen + slacklen);
> - do_cleanups (back_to);
alloca is unsafe, and we prefer to use xmalloc/cleanup+xfree.
> 0002-tilegx-tdep.c-tilegx_analyze_prologue-fix-prologue-a.patch
>
>
> From 028cafa74fe2601ebf13e9a64e6de90e891cbed8 Mon Sep 17 00:00:00 2001
> From: Jiong Wang<jiwang@tilera.com>
> Date: Thu, 6 Dec 2012 12:15:32 +0800
> Subject: [PATCH 2/5] * tilegx-tdep.c (tilegx_analyze_prologue): fix
> prologue analysis overflow bug
>
> when setting breakpoint by function name, if that function
> is in .so and that .so is not loaded yet, then gdb will do
> partial name match, which will match the corresponding plt
> stub entry. We should take this situation into account when
> doing prologue analysis.
> ---
> gdb/tilegx-tdep.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
> index 2f7f253..93969c9 100644
> --- a/gdb/tilegx-tdep.c
> +++ b/gdb/tilegx-tdep.c
> @@ -432,8 +432,22 @@ tilegx_analyze_prologue (struct gdbarch* gdbarch,
>
> status = safe_frame_unwind_memory (next_frame, instbuf_start,
> instbuf, instbuf_size);
> - if (status == 0)
> - memory_error (status, next_addr);
> + if (status == 0) {
> + /* fix gdb.base/gdb1250
> + * breakpoint is set before dynamic library loaded, thus gdb
> + * does a partial symbol name finding and sets the breakpoint
> + * in the plt stub. our 32-bundle prefetch window is too large
> + * for this situation to cause a memory access error.
> + * For plt stub, we just need to return directly.
> + *
> + * x86 does not have this problem, because the first instruction
> + * in their plt stub is jump, which ends the analysis also.
> + */
The comment style is not gnu style. The problems looks about plt. I
find tilegx port doesn't have a plt stub unwinder. I am not sure
creating a plt stub unwinder can fix this problem, but it should fix
other fails in testsuite.
> + if (strcmp(find_pc_section(instbuf_start)->the_bfd_section->name,
> + ".plt") == 0)
> + return instbuf_start;
> + memory_error (status, next_addr);
> + }
> }
>
> reverse_frame_valid = 0;
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/tilegx] tilegx bug fixes & improvements
2012-12-06 9:39 ` Yao Qi
@ 2012-12-06 15:20 ` Jiong Wang
2012-12-07 2:40 ` Yao Qi
2012-12-07 2:42 ` Joel Brobecker
0 siblings, 2 replies; 6+ messages in thread
From: Jiong Wang @ 2012-12-06 15:20 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches, Walter Lee, palves
[-- Attachment #1: Type: text/plain, Size: 1954 bytes --]
Hi Yao,
>> - if (alignlen & 0x4)
>> - stack_dest -= 4;
>> + stack_dest = (stack_dest + 7) & ~0x7;
>
> Your can use function 'align_up'.
fixed, and I do make a mistake here, should be align_down.
>> /* Now write data to the stack. The stack grows downwards. */
>> stack_dest -= typelen + slacklen;
>> write_memory (stack_dest, val, typelen + slacklen);
>> - do_cleanups (back_to);
>
> alloca is unsafe, and we prefer to use xmalloc/cleanup+xfree.
fixed.
>> + if (status == 0) {
>> + /* fix gdb.base/gdb1250
>> + * breakpoint is set before dynamic library loaded, thus
>> gdb
>> + * does a partial symbol name finding and sets the
>> breakpoint
>> + * in the plt stub. our 32-bundle prefetch window is too
>> large
>> + * for this situation to cause a memory access error.
>> + * For plt stub, we just need to return directly.
>> + *
>> + * x86 does not have this problem, because the first
>> instruction
>> + * in their plt stub is jump, which ends the analysis also.
>> + */
>
> The comment style is not gnu style.
fixed the comment style.
> The problems looks about plt. I find tilegx port doesn't have a plt
> stub unwinder. I am not sure creating a plt stub unwinder can fix
> this problem,
I am not quite understand the usage of plt stub unwinder here. i386
do not append plt stub unwinder also. from my understanding, those plt
stub can be unwinded as normal frame, like a leaf function, or have I
misunderstood something?
> but it should fix other fails in testsuite.
the left failures are mostly about type print issues under gdb.cp
and gdb.python
attachment is the patch to fix above problems
thanks for feedback
---
Regards,
Jiong. Wang
Tilera Corporation
--
Regards,
Salad
office: +86-010-82825915, ext:653
cell: +86-13810021970
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-glitches.patch --]
[-- Type: text/plain; charset="gb18030"; name="fix-glitches.patch", Size: 2723 bytes --]
diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
index f7e00d7..903bf20 100644
--- a/gdb/tilegx-tdep.c
+++ b/gdb/tilegx-tdep.c
@@ -324,23 +324,27 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
}
/* Align SP. */
- stack_dest = (stack_dest + 7) & ~0x7;
+ stack_dest = align_down(stack_dest, 8);
/* Loop backwards through remaining arguments and push them on
the stack, word aligned. */
for (j = nargs - 1; j >= i; j--)
{
gdb_byte *val;
+ struct cleanup *back_to;
+ const gdb_byte *contents = value_contents (args[j]);
typelen = TYPE_LENGTH (value_enclosing_type (args[j]));
slacklen = ((typelen + 7) & (~7)) - typelen;
- val = alloca (typelen + slacklen);
- memcpy (val, value_contents (args[j]), typelen);
+ val = xmalloc (typelen + slacklen);
+ back_to = make_cleanup (xfree, val);
+ memcpy (val, contents, typelen);
memset (val + typelen, 0, slacklen);
/* Now write data to the stack. The stack grows downwards. */
stack_dest -= typelen + slacklen;
write_memory (stack_dest, val, typelen + slacklen);
+ do_cleanups (back_to);
}
/* Add 2 double words for linkage space to the stack. */
@@ -434,17 +438,16 @@ tilegx_analyze_prologue (struct gdbarch* gdbarch,
instbuf, instbuf_size);
if (status == 0) {
/* fix gdb.base/gdb1250
- * breakpoint is set before dynamic library loaded, thus gdb
- * does a partial symbol name finding and sets the breakpoint
- * in the plt stub. our 32-bundle prefetch window is too large
- * for this situation to cause a memory access error.
- * For plt stub, we just need to return directly.
- *
- * x86 does not have this problem, because the first instruction
- * in their plt stub is jump, which ends the analysis also.
+ breakpoint is set before dynamic library loaded, thus gdb
+ does a partial symbol name finding and sets the breakpoint
+ in the plt stub. our 32-bundle prefetch window is too large
+ for this situation to cause a memory access error.
+ For plt stub, we just need to return directly.
+
+ x86 does not have this problem, because the first instruction
+ in their plt stub is jump, which ends the analysis also.
*/
- if (strcmp(find_pc_section(instbuf_start)->the_bfd_section->name,
- ".plt") == 0)
+ if (in_plt_section (instbuf_start, NULL))
return instbuf_start;
memory_error (status, next_addr);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/tilegx] tilegx bug fixes & improvements
2012-12-06 15:20 ` Jiong Wang
@ 2012-12-07 2:40 ` Yao Qi
2012-12-07 2:42 ` Joel Brobecker
1 sibling, 0 replies; 6+ messages in thread
From: Yao Qi @ 2012-12-07 2:40 UTC (permalink / raw)
To: Jiong Wang; +Cc: gdb-patches, Walter Lee, palves
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb18030"; format=flowed, Size: 1731 bytes --]
On 12/06/2012 11:19 PM, Jiong Wang wrote:
>> The problems looks about plt. I find tilegx port doesn't have a plt
>> >stub unwinder. I am not sure creating a plt stub unwinder can fix
>> >this problem,
> I am not quite understand the usage of plt stub unwinder here. i386
> do not append plt stub unwinder also. from my understanding, those plt
> stub can be unwinded as normal frame, like a leaf function, or have I
> misunderstood something?
>
I am not familiar with i386 port. plt stub unwinder is helpful to
identify current pc is within the plt stub and dynamic resolver, IIRC.
It is useful when user type command 'n' to step over function 'printf',
for example, otherwise, gdb will stop inside printf instead of the next
line of printf, which is expected.
>> >but it should fix other fails in testsuite.
> the left failures are mostly about type print issues under gdb.cp
> and gdb.python
>
plt stub unwinder may not help in this case.
> attachment is the patch to fix above problems
>
Thanks for the fix. We don't post a delta-patch on top of the reviewed
patch. If you updated your patch to address review comments, please
post the new one instead of the delta part (and one mail for one patch).
> diff --git a/gdb/tilegx-tdep.c b/gdb/tilegx-tdep.c
> index f7e00d7..903bf20 100644
> --- a/gdb/tilegx-tdep.c
> +++ b/gdb/tilegx-tdep.c
> @@ -324,23 +324,27 @@ tilegx_push_dummy_call (struct gdbarch *gdbarch,
> }
>
> /* Align SP. */
> - stack_dest = (stack_dest + 7) & ~0x7;
> + stack_dest = align_down(stack_dest, 8);
^ A space is missing.
or you can use 'tilegx_frame_align', which was used before you change.
--
Yao (ÆëÒ¢)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/tilegx] tilegx bug fixes & improvements
2012-12-06 15:20 ` Jiong Wang
2012-12-07 2:40 ` Yao Qi
@ 2012-12-07 2:42 ` Joel Brobecker
2012-12-07 3:12 ` Jiong Wang
1 sibling, 1 reply; 6+ messages in thread
From: Joel Brobecker @ 2012-12-07 2:42 UTC (permalink / raw)
To: Jiong Wang; +Cc: Yao Qi, gdb-patches, Walter Lee, palves
> >alloca is unsafe, and we prefer to use xmalloc/cleanup+xfree.
>
> fixed.
Just to be more precise, we avoid the use of alloca when inside
a loop (or inside a function that's typically called inside a loop),
in order to avoid blowing the stack. But using alloca for resonably-
sized objects is otherwise fine.
> attachment is the patch to fix above problems
I think you need to fold this patch into the previous patches you
submitted, and then make sure you re-submit them, one patch per
email - making sure you update the ChangeLog files when necessary.
I noticed that some of your commits did not have a subject. And
it would be good if each commit explained precisely what it improves.
Take a look at how each patch in the following series of 4 patches
has been submitted for instance:
http://www.sourceware.org/ml/gdb-patches/2012-11/msg00854.html
(links to referenced emails are at the bottom of the page).
I apologize in advance for the comments below, as they seem like
nit-picking. But they are about coding style, and trying to be
consistent within a project is important.
> - stack_dest = (stack_dest + 7) & ~0x7;
> + stack_dest = align_down(stack_dest, 8);
You are missing a space before the '('.
> if (status == 0) {
Our style is to put the brace on the next line, thus:
if (status == 0)
{
> /* fix gdb.base/gdb1250
Another nit: Sentences start with a capital letter and end with
a period. Thus:
/* Fix gdb.base/gdb1250.
I am not too fond of references to the testsuite, however. Is that
necessary? I don't think people grep the sources when they change
anything in the testsuite. As long as you describe why you are
making a change, I don't see a need for saying what testcase you
are fixing.
I haven't really looked at the contents of the patch, only the style.
We'll get to that when you resubmit them.
> - * breakpoint is set before dynamic library loaded, thus gdb
> - * does a partial symbol name finding and sets the breakpoint
> - * in the plt stub. our 32-bundle prefetch window is too large
> - * for this situation to cause a memory access error.
> - * For plt stub, we just need to return directly.
> - *
> - * x86 does not have this problem, because the first instruction
> - * in their plt stub is jump, which ends the analysis also.
> + breakpoint is set before dynamic library loaded, thus gdb
> + does a partial symbol name finding and sets the breakpoint
> + in the plt stub. our 32-bundle prefetch window is too large
> + for this situation to cause a memory access error.
> + For plt stub, we just need to return directly.
> +
> + x86 does not have this problem, because the first instruction
> + in their plt stub is jump, which ends the analysis also.
> */
> - if (strcmp(find_pc_section(instbuf_start)->the_bfd_section->name,
> - ".plt") == 0)
> + if (in_plt_section (instbuf_start, NULL))
> return instbuf_start;
> memory_error (status, next_addr);
> }
--
Joel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH/tilegx] tilegx bug fixes & improvements
2012-12-07 2:42 ` Joel Brobecker
@ 2012-12-07 3:12 ` Jiong Wang
0 siblings, 0 replies; 6+ messages in thread
From: Jiong Wang @ 2012-12-07 3:12 UTC (permalink / raw)
To: Joel Brobecker; +Cc: Yao Qi, gdb-patches, Walter Lee, palves
Hi Joel & Yao,
thanks for your time for careful review & suggestions.
I will re-organize the patches, re-test, then re-submit
---
Regards,
Jiong
On 12/07/2012 10:41 AM, Joel Brobecker wrote:
>>> alloca is unsafe, and we prefer to use xmalloc/cleanup+xfree.
>> fixed.
> Just to be more precise, we avoid the use of alloca when inside
> a loop (or inside a function that's typically called inside a loop),
> in order to avoid blowing the stack. But using alloca for resonably-
> sized objects is otherwise fine.
>
>> attachment is the patch to fix above problems
> I think you need to fold this patch into the previous patches you
> submitted, and then make sure you re-submit them, one patch per
> email - making sure you update the ChangeLog files when necessary.
>
> I noticed that some of your commits did not have a subject. And
> it would be good if each commit explained precisely what it improves.
>
> Take a look at how each patch in the following series of 4 patches
> has been submitted for instance:
> http://www.sourceware.org/ml/gdb-patches/2012-11/msg00854.html
> (links to referenced emails are at the bottom of the page).
>
> I apologize in advance for the comments below, as they seem like
> nit-picking. But they are about coding style, and trying to be
> consistent within a project is important.
>
>> - stack_dest = (stack_dest + 7) & ~0x7;
>> + stack_dest = align_down(stack_dest, 8);
> You are missing a space before the '('.
>
>> if (status == 0) {
> Our style is to put the brace on the next line, thus:
>
> if (status == 0)
> {
>
>> /* fix gdb.base/gdb1250
> Another nit: Sentences start with a capital letter and end with
> a period. Thus:
>
> /* Fix gdb.base/gdb1250.
>
> I am not too fond of references to the testsuite, however. Is that
> necessary? I don't think people grep the sources when they change
> anything in the testsuite. As long as you describe why you are
> making a change, I don't see a need for saying what testcase you
> are fixing.
>
> I haven't really looked at the contents of the patch, only the style.
> We'll get to that when you resubmit them.
>
>> - * breakpoint is set before dynamic library loaded, thus gdb
>> - * does a partial symbol name finding and sets the breakpoint
>> - * in the plt stub. our 32-bundle prefetch window is too large
>> - * for this situation to cause a memory access error.
>> - * For plt stub, we just need to return directly.
>> - *
>> - * x86 does not have this problem, because the first instruction
>> - * in their plt stub is jump, which ends the analysis also.
>> + breakpoint is set before dynamic library loaded, thus gdb
>> + does a partial symbol name finding and sets the breakpoint
>> + in the plt stub. our 32-bundle prefetch window is too large
>> + for this situation to cause a memory access error.
>> + For plt stub, we just need to return directly.
>> +
>> + x86 does not have this problem, because the first instruction
>> + in their plt stub is jump, which ends the analysis also.
>> */
>> - if (strcmp(find_pc_section(instbuf_start)->the_bfd_section->name,
>> - ".plt") == 0)
>> + if (in_plt_section (instbuf_start, NULL))
>> return instbuf_start;
>> memory_error (status, next_addr);
>> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-12-07 3:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-06 7:00 [PATCH/tilegx] tilegx bug fixes & improvements Jiong Wang
2012-12-06 9:39 ` Yao Qi
2012-12-06 15:20 ` Jiong Wang
2012-12-07 2:40 ` Yao Qi
2012-12-07 2:42 ` Joel Brobecker
2012-12-07 3:12 ` Jiong Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox