Re: [PATCHv2,Hurd] Fix deallocation after proc_getprocinfo call
2014-10-02 Samuel Thibault samuel.thiba...@ens-lyon.org * gdb/gnu-nat.c (inf_validate_procinfo): Multiply the number of elements pi_len by the size of the elements before calling vm_deallocate. (inf_validate_task_sc): Likewise, and properly deallocate the noise array. Thank you, Samuel. The patch has now been pushed. -- Joel
Re: [PATCH,Hurd] Fix deallocation after proc_getprocinfo call
- vm_deallocate (mach_task_self (), (vm_address_t) pi, pi_len); + vm_deallocate (mach_task_self (), (vm_address_t) pi, pi_len * sizeof (*(procinfo_t) 0)); Suggest using sizeof (struct procinfo), which I think is better than dereferencing a NULL pointer. This is based on guessing that type procinfo_t is a pointer to struct procinfo, as suggested by the code in inf_validate_procinfo. Not, that is not the same: struct procinfo has an open array at its end (threadinfos[0]), and thus the actually allocated size is variable. OK. I don't know the code well enough to make any better suggestion. The above does look unusual to me, but if it works and seems to be the only correct way, let's go with that. Should I push your patch? -- Joel
Re: [PATCH,Hurd] Fix deallocation after proc_getprocinfo call
Hello Samuel,
On Sun, Nov 02, 2014 at 04:25:37PM +0100, Samuel Thibault wrote:
2014-10-02 Samuel Thibault samuel.thiba...@ens-lyon.org
* gdb/gnu-nat.c (inf_validate_procinfo): Multiply the number of
elements pi_len by the size of the elements before calling
vm_deallocate.
(inf_validate_task_sc): Likewise, and properly deallocate the
noise array.
Again, sorry about the late review...
I only have a few minor comments, almost trivial in nature. In
the ChangeLog entry above, watch out that the last 2 lines are
indented using spaces intead of tabs.
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index d17a750..c571190 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -804,7 +804,7 @@ inf_validate_procinfo (struct inf *inf)
inf-nomsg = !!(pi-state PI_NOMSG);
if (inf-nomsg)
inf-traced = !!(pi-state PI_TRACED);
- vm_deallocate (mach_task_self (), (vm_address_t) pi, pi_len);
+ vm_deallocate (mach_task_self (), (vm_address_t) pi, pi_len * sizeof
(*(procinfo_t) 0));
The line is too long (soft limit is 74 characters, hard limit is 80).
Suggest using sizeof (struct procinfo), which I think is better
than dereferencing a NULL pointer. This is based on guessing that
type procinfo_t is a pointer to struct procinfo, as suggested by
the code in inf_validate_procinfo.
if (noise_len 0)
vm_deallocate (mach_task_self (), (vm_address_t) noise, noise_len);
}
@@ -844,9 +844,9 @@ inf_validate_task_sc (struct inf *inf)
suspend_count = pi-taskinfo.suspend_count;
- vm_deallocate (mach_task_self (), (vm_address_t) pi, pi_len);
+ vm_deallocate (mach_task_self (), (vm_address_t) pi, pi_len * sizeof
(*(procinfo_t) 0));
Same as above.
if (noise_len 0)
-vm_deallocate (mach_task_self (), (vm_address_t) pi, pi_len);
+vm_deallocate (mach_task_self (), (vm_address_t) noise, noise_len);
if (inf-task-cur_sc suspend_count)
{
Thank you,
--
Joel
Re: [PATCHv3,Hurd] Add hardware watch support
Seeing a submission to gdb getting stuck on missing line breaks got me a bit on my nerves. Fortunately it wasn't only about that, but also important changes, so I carried on, but having to resubmit only for missing spaces or new lines would have been really hard to me, since I don't really plan to submit many patches to gdb, and I know I'll always make this kind of mistakes since I'm submitting patches to a lot of various projects with very differing coding styles. I understand the fustration, and I certainly would agree if you said that GDB is very particular with its CS. Aiming for a consistent coding style is important for long-term maintainability, though, which is why we almost systematically mention violations we see. If it makes you feel any better, we make mistakes too, sometimes, and have to re-edit as a result. One thing I will say, however, is that I often let go of minor violations, and only mention them if there are other adjustments I'd like to request in the same area. And I know others can be the same. So we try not be be overly picky (you may not believe me on that one ;-)!). As Sergio mentioned, we do not have a script we can use to check coding-style violations. We would very much like to have one, but no one really sent anything so far. What I can do to help is being your CS corrector - just send me your patch privately before submitting, and I will fix all CS violations for you. Another important part I can see contributing to the fustration is review delays. I can see how trivial requests can be extra fustrating if you feel like your patch is being delayed quite a bit because each review cycle is very long. You can help with that by pinging us. The accepted practice is to ping after a week or two, and every week thereafter. -- Joel
Re: [PATCHv3,Hurd] Add hardware watch support
2014-09-06 Samuel Thibault samuel.thiba...@ens-lyon.org
Add hardware watch support to gnu-i386 platform.
* gdb/gdb/gnu-nat.c (inf_threads): New function.
* gdb/gdb/gnu-nat.h (inf_threads_ftype): New type.
(inf_threads): New declaration.
* gdb/gdb/i386gnu-nat.c: Include x86-nat.h and inf-child.h.
[i386_DEBUG_STATE] (i386_gnu_dr_get, i386_gnu_dr_set,
i386_gnu_dr_set_control_one, i386_gnu_dr_set_control,
i386_gnu_dr_set_addr_one, i386_gnu_dr_set_addr, i386_gnu_dr_get_reg,
i386_gnu_dr_get_addr, 386_gnu_dr_get_status, i386_gnu_dr_get_control):
New functions
(reg_addr): New structure.
(_initialize_i386gnu_nat) [i386_DEBUG_STATE]: Initialize hardware i386
debugging register hooks.
In addition to Sergio's comments, I noticed:
You forgot the comment I made about having the function documented
at only one location, and the contents of that documentat. For easy
reference, here are my comments again:
| Use...
|
| /* See gnu-nat.h. */
|
| ... instead. This is a fairly trivial comment in this case, so
| not biggie, but the idea is that want to avoid duplicating
| documentation in order to avoid having one of them being out
| of sync.
|
| And please also make sure to always have an empty line before
| function documentation and start of implementation.
|
| void
| diff --git a/gdb/gnu-nat.h b/gdb/gnu-nat.h
| index 8e949eb..011c38c 100644
| --- a/gdb/gnu-nat.h
| +++ b/gdb/gnu-nat.h
| @@ -29,6 +29,11 @@ extern struct inf *gnu_current_inf;
| /* Converts a GDB pid to a struct proc. */
| struct proc *inf_tid_to_thread (struct inf *inf, int tid);
|
| +typedef void (inf_threads_ftype) (struct proc *thread);
| +
| +/* Iterate F over threads. */
|
| Suggest:
|
| /* Call F for every thread in inferior INF. */
|
| This addresses two issues: Iterate F sounds odd, but perhaps
| that's because English is not my native language; but also,
| it also documents what INF is.
+static void i386_gnu_dr_set_control_one (struct proc *thread, void *arg)
+{
For the function implementations, the name of the function should
be at the first column on the next line. This is a GNU Coding Style
(GCS) requirement, and allows easy searching of a function's body by
grepping for ^FUNCTION_NAME. I also think it helps having the name
of the function for each hunk in diff -p (not completely sure about
that one).
+ unsigned long *control = arg;
+ struct i386_debug_state regs;
+ i386_gnu_dr_get (regs, thread);
I just wanted to add that Sergio's request to add an empty line after
the variable declarations is actually a rule in the GDB project.
For the record, our coding standards are documented at:
https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards
It's not complete yet, so do not be too surprise if we come up with
things that you haven't seen there or in the GCS. Do call us on it,
though, and we will update the doc.
+struct reg_addr {
+ int regnum;
+ CORE_ADDR addr;
+};
While touching this code, we tend to prefer it for struct
definitions if the opening curly brace is at the start of
the next line, please.
struct reg_addr
{
int regnum;
CORE_ADDR addr;
};
+static void i386_gnu_dr_set_addr_one (struct proc *thread, void *arg)
Same as above.
+ struct reg_addr reg_addr;
+ gdb_assert (DR_FIRSTADDR = regnum regnum = DR_LASTADDR);
Missing empty line between var declaration and rest of code.
--
Joel
Re: [PATCHv4,Hurd] Add hardware watch support
Hello Samuel, On Fri, Sep 12, 2014 at 08:29:11PM +0200, Samuel Thibault wrote: 2014-09-06 Samuel Thibault samuel.thiba...@ens-lyon.org Add hardware watch support to gnu-i386 platform. This allows to get the watch command keep native code execution. * gdb/gdb/gnu-nat.c (inf_threads): New function. * gdb/gdb/gnu-nat.h (inf_threads_ftype): New type. (inf_threads): New declaration. * gdb/gdb/i386gnu-nat.c: Include x86-nat.h and inf-child.h. [i386_DEBUG_STATE] (i386_gnu_dr_get, i386_gnu_dr_set, i386_gnu_dr_set_control_one, i386_gnu_dr_set_control, i386_gnu_dr_set_addr_one, i386_gnu_dr_set_addr, i386_gnu_dr_get_reg, i386_gnu_dr_get_addr, 386_gnu_dr_get_status, i386_gnu_dr_get_control): New functions (reg_addr): New structure. (_initialize_i386gnu_nat) [i386_DEBUG_STATE]: Initialize hardware i386 debugging register hooks. This is now OK. I double-checked that Samuel does have a copyright assignment on file, so Thomas is good to go for pushing the patch in if he approves as well. If Sergio has any further comment on the patch, we can address them as followups. Thank you, -- Joel
Re: [PATCHv3,Hurd] Add hardware watch support
Many thanks for persisting with this patch. I have to say I'm almost about to give up with submitting it. I would be very interested in hearing your honest feedback on this. I know it took a long time, and we're not always very responsive, but we try our best. If we could hear what made you feel this way, I would like to try to see if there are any ways we can improve the situation for you. -- Joel
Re: [PATCHv2,Hurd] Add hardware watch support
Samuel,
I only have small comments.
First, the ChangeLog entry:
2014-09-06 Samuel Thibault samuel.thiba...@ens-lyon.org
Add hardware watch support to gnu-i386 platform.
* gdb/gdb/gnu-nat.c (inf_threads): New function.
* gdb/gdb/gnu-nat.h (inf_threads_ftype): New type.
(inf_threads): New declaration.
* gdb/gdb/i386gnu-nat.c: Include x86-nat.h and inf-child.h
Missing period at the end of this sentence.
[i386_DEBUG_STATE] (i386_gnu_dr_get, i386_gnu_dr_set,
It looks like this is improperly indented (spaces instead of tabs?).
i386_gnu_dr_set_control, i386_gnu_dr_set_addr, i386_gnu_dr_get_reg,
i386_gnu_dr_get_addr, 386_gnu_dr_get_status, i386_gnu_dr_get_control):
New functions
[i386_DEBUG_STATE] (_initialize_i386gnu_nat): Initialize hardware i386
I would swap the i386_DEBUG_STATE and _initialize_i386gnu_nat.
You're inside _initialize_i386gnu_nat, not the other around.
debugging register hooks.
diff --git a/gdb/gnu-nat.c b/gdb/gnu-nat.c
index c8164d6..2d7c32c 100644
--- a/gdb/gnu-nat.c
+++ b/gdb/gnu-nat.c
@@ -983,6 +983,16 @@ inf_port_to_thread (struct inf *inf, mach_port_t port)
return 0;
}
+/* Iterate F over threads. */
Use...
/* See gnu-nat.h. */
... instead. This is a fairly trivial comment in this case, so
not biggie, but the idea is that want to avoid duplicating
documentation in order to avoid having one of them being out
of sync.
And please also make sure to always have an empty line before
function documentation and start of implementation.
void
diff --git a/gdb/gnu-nat.h b/gdb/gnu-nat.h
index 8e949eb..011c38c 100644
--- a/gdb/gnu-nat.h
+++ b/gdb/gnu-nat.h
@@ -29,6 +29,11 @@ extern struct inf *gnu_current_inf;
/* Converts a GDB pid to a struct proc. */
struct proc *inf_tid_to_thread (struct inf *inf, int tid);
+typedef void (inf_threads_ftype) (struct proc *thread);
+
+/* Iterate F over threads. */
Suggest:
/* Call F for every thread in inferior INF. */
This addresses two issues: Iterate F sounds odd, but perhaps
that's because English is not my native language; but also,
it also documents what INF is.
+/* Set DR_CONTROL to ADDR in all threads. */
+
+static void
+i386_gnu_dr_set_control (unsigned long control)
The documentation seems out of sync with the function's prototype.
+{
+ void f (struct proc *thread)
+ {
+struct i386_debug_state regs;
+i386_gnu_dr_get (regs, thread);
+regs.dr[DR_CONTROL] = control;
+i386_gnu_dr_set (regs, thread);
+ }
Nested functions are not allowed. We'd like GCC to remain
compilable using non-GCC C90 compilers.
+/* Set address REGNUM (zero based) to ADDR in all threads. */
+
+static void
+i386_gnu_dr_set_addr (int regnum, CORE_ADDR addr)
+{
+ void f (struct proc *thread)
+ {
+struct i386_debug_state regs;
+i386_gnu_dr_get (regs, thread);
+regs.dr[regnum] = addr;
+i386_gnu_dr_set (regs, thread);
+ }
Same here, no nested function, please.
--
Joel
Re: [PATCH 1/2] Port gdbserver to GNU/Hurd
I'm actually very excited to see gdb and gdbserver sharing a single target backend, even if we still have many wrinkles in the interfaces to iron out! Mee too! -- Joel
Re: [PATCH,HURD] Fix reading core
* gdb/i386gnu-nat.c (CREG_OFFSET): New macro. (creg_offset): New array. (CREG_ADDR): Use creg_offset instead of reg_offset. Did anyone review this? I don't know GNU/Hurd, but the patch looks very plausible. Do you have write access to the GDB repository? I don't. If not, do you have copyright assignment papers on file? It's on file now. OK - if you have other patches in the pipeline, let's give you write access to the repository as soon as this patch is approved. I am not clear whether I should re-review, or let Thomas do it. Thank you, -- Joel
Re: [PATCH,HURD] Fix reading core
The i386 GNU/Hurd ELF core format actually follows the uaccess gregset_t
array format, not the Mach thread state format. This fixes gdb reading
it.
* gdb/i386gnu-nat.c (CREG_OFFSET): New macro.
(creg_offset): New array.
(CREG_ADDR): Use creg_offset instead of reg_offset.
Did anyone review this? I don't know GNU/Hurd, but the patch looks
very plausible.
Do you have write access to the GDB repository? If not, do you have
copyright assignment papers on file? This patch is small enough that
we can take it as a tiny patch, but if you think you're going to
send more patches, we should probably get you started on those.
The ChangeLog entry above needs to be indented properly (just to be
sure). Another tiny style nitpick below...
--- a/gdb/i386gnu-nat.c.original 2013-02-11 00:46:02.0 +
+++ b/gdb/i386gnu-nat.c 2013-02-11 00:48:09.0 +
@@ -56,8 +56,21 @@
REG_OFFSET (ds), REG_OFFSET (es), REG_OFFSET (fs), REG_OFFSET (gs)
};
+/* Offset to the greg_t location where REG is stored. */
+#define CREG_OFFSET(reg) (REG_##reg * 4)
+
+/* At CREG_OFFSET[N] is the offset to the greg_t location where
+ the GDB register N is stored. */
+static int creg_offset[] =
+{
+ CREG_OFFSET (EAX), CREG_OFFSET (ECX), CREG_OFFSET (EDX), CREG_OFFSET (EBX),
+ CREG_OFFSET (UESP), CREG_OFFSET (EBP), CREG_OFFSET (ESI), CREG_OFFSET
(EDI),
+ CREG_OFFSET (EIP), CREG_OFFSET (EFL), CREG_OFFSET (CS), CREG_OFFSET (SS),
+ CREG_OFFSET (DS), CREG_OFFSET (ES), CREG_OFFSET (FS), CREG_OFFSET (GS)
+};
Unless it was done on purpose, we try to limit the size of lines
to 70 characters, only extending it to up to 80 when it makes
a difference
+
#define REG_ADDR(state, regnum) ((char *)(state) + reg_offset[regnum])
-#define CREG_ADDR(state, regnum) ((const char *)(state) + reg_offset[regnum])
+#define CREG_ADDR(state, regnum) ((const char *)(state) +
creg_offset[regnum])
/* Get the whole floating-point state of THREAD and record the values
--
Joel
