Implement ddb/db_elf.c

URL:
<>

Summary: Implement ddb/db_elf.c
Project: The GNU Hurd
Submitted by: sthibaul
Submitted on: mardi 20.02.2007 * 01:52
Category: GNU Mach
Should Start : mardi 20.02.2007 * 00:00
Should be Finished on: mardi 20.02.2007 * 00:00
Priority: 1 - Later
Status: None
Privacy: Public
Percent Complete: 0%
Assigned to: None
/Closed:
Discussion Lock: Any
Planned Release: None
Effort: 0.00
Wiki-like text discussion box:



Details:

For getting useful information from ddb's trace command, ELF symbol lookup
should be implemented in ddb/db_elf.c






Reply to this item at:

<>


Message © via/par Savannah
http://savannah.gnu.org/




Bug-hurd mailing list
Bug-hurd (AT) gnu (DOT) org

Andrei Barbu, le Fri 09 May 2008 15:38:25 +0000, a :
Two patches that add this.

Cool, definitely a helpful thing :)

I will most probably not have the time to review them until at least one
week, however.

Samuel

Hello,

I haven't reviewed db_elf.c itself, thus not replying on the tracker.

It generally looks good (I've commited the symbol_values already), just
a few points:

- take care of indentation. Try to respect the same existing indentation
(yes it's not homogeneous, follow it per-file). For instance, in

- use -p option of diff.
- I'm not sure we want to instroduce the PSIX types. Can't we just use
the Mach types in the ELF headers? It looks like there aren't so many
occurences anyway.
- I would have put __CNCAT in the ELF header, as it's the only place
where it is used. helper macros aren't used.
- in model_dep.c you can factorize the test for kern_sym_start.
- please provide a GNU-style ChangeLog.

Again, thanks for working on that, it'll definitely be useful :)

Samuel

Hello,

Andrei Barbu, le Sun 25 May 2008 02:07:36 -0400, a :
- I'm not sure we want to instroduce the PSIX types. Can't we just use
the Mach types in the ELF headers? It looks like there aren't so many
occurences anyway.
I'd prefer to keep the headers unchaged.

, then put them into sys/types.h, since that's where PSIX says they
are :)

- I would have put __CNCAT in the ELF header, as it's the only place
where it is used. helper macros aren't used.
Same as above.

Then another place too, it really doesn't belong to vm_types.h :)

Samuel

, then put them into sys/types.h, since that's where PSIX says they
are :)
>- I would have put __CNCAT in the ELF header, as it's the only place
>where it is used. helper macros aren't used.
>Same as above.
>
Then another place too, it really doesn't belong to vm_types.h :)




Andrei

Hello,

Looks good (quite straight forward actually), except the coding style of
db_elf.c. It'd be good to keep the same as db_aout.c for instance, for
at least local style coherency :)

Samuel

Andrei Barbu, le Tue 27 May 2008 19:02:18 +0000, a :
Additional Item Attachment, task #6537 (project hurd):

File name: changelog-elf-1.patch Size:1 KB

, you make the same mistake as I did some time ago when I started
working on GNU projects :)

GNU style changelog are not about why you make changes, but about what
changes you make. Putting a "Add ELF symbol lookup" header is useful,
but no need for more. The rest should just be e.g.

* ddb/db_elf.c: New file.

* ddb/db_sym.h (SYMTAB_ELF): New macro
(SYMTAB_MACHDEP): Update macro value
(symtab_type): Return SYMTAB_ELF instead of SYMTAB_AUT.
* i386/i386at/model_dep.c: Include <ddb/elf.hand
* <machine/vm_param.h>.
(c_boot_entry): If ELF debugging information is available, call
elf_db_sym_init. Call aout_db_sym_init only when AUT debugging
information is available.


I.e. yes, writing a GNU ChangeLog is not about justifications, but just
stupidly summing up the effect of the patch: the goal is to tell what
the patch does, and in particular the name of the things that have
changed (functions, macros, etc). It helps when looking for things that
happened with a function for instance. Please really read the ChangeLog
part of the GNU Coding Style, and the existing ChangeLog for examples.

Samuel