Mail Archives: djgpp-workers/2003/04/22/19:18:57
Hello.
Charles Sandmann wrote:
> I've put the modules for the updated dxe3 code in:
>
> ftp://clio.rice.edu/djgpp/dxe3.zip
> http://clio.rice.edu/djgpp/dxe3.zip
[snip]
I've just taken a look at the code. I looked at this particular version of the
ZIP:
iolanthe:~/src/djgpp-dxe3/dxe3-20030421 =] ls -l /zips/dxe3-20030421.zip
-rw-rw-r-- 1 rich rich 61522 Apr 20 22:54
/zips/dxe3-20030421.zip
I have some comments on specific files:
src/dxe/dxe3.txt:
* This mentions dxe3.h at one point. The only file I can see in the archive is
include/sys/dxe.h. Should it refer to that instead? If this file is historic,
i.e. just a copy of the one from the DXE3, then ignore this comment. If it's
supposed to be up-to-date regarding DJGPP CVS, then I think it should be
updated.
* Later: Ah, I see, dxegen.txi is now the definitive documentation. Maybe a
note should be added at the top of dxe3.txt, to say that.
src/dxe/dxegen.txi (some of these comments probably apply to dxe3.txt too):
* -o is listed twice in the syntax for dxe3gen: explicitly and as an option.
* "(where MODNAME stands for the name of your dynamic module)" - I think
MODNAME should be in @code{}.
* "you CANNOT have exported data inside the library" - CANNOT should be in
@emph{} or @strong{} - probably @emph.
* It's better to use @dots{} rather than '...'.
* "Unix apps" -> "Unix applications"
* "For plain C you will have to use the GNU C extension (which is also
supported on all platforms where GNU C is available, but you are restricted
with the compiler):" - "the GNU C extension" -> "GNU C extensions"; I don't
think the text in brackets is needed - we've already said it's a GNU
C-specific extension.
* Function names in @code{} don't need brackets - we're not calling the
functions. E.g.: @code{strlen()} -> @code{strlen}.
* There are still some "-*-" list markers in the text. Should these be
removed, now you format the list using @itemize?
* "The better way is to leave the symbols unresolved" - either it's "a better
way" or it's "the best way".
* In the example that does "dlopen ("moduleA.dxe", RTLD_GLOBAL);", the example
should be split in two, so the text in the middle is formatted normally.
* "@strong{Import libraries}" - shouldn't this be an @section? Similarly for
other @strong{} usages that are headings.
* "(see http://crystal.sourceforge.net)" - use @uref{}, so that the URL
appears as a link, when the texinfo is converted to HTML format using
makeinfo. Similarly for the other web addresses.
* Do the authors really want their e-mail addresses in the texinfo
documentation?
src/libc/dxe/dlregsym.c:
* If the realloc fails, it loses the pointer to the original symbol table
_dl_symtabs. realloc does not touch the original memory, if it cannot allocate
a new block of the appropriate size. So the original is still valid. Perhaps
it should leave the table as it is and just fail, when it cannot reallocate
the memory. E.g.:
_old_dl_nmaxsymtab = _dl_nmaxsymtab;
_old_dl_symtabs = _dl_symtabs;
_dl_nmaxsymtab += 8;
_dl_symtabs = realloc(...);
if (_dl_symtabs == NULL) {
_dl_nmaxsymtab = _old_dl_nmaxsymtab;
_dl_symtabs = _old_dl_symtabs;
return 1;
}
src/libc/dxe/dlopen.c:
* DJGPP has a realpath function now. REALPATH should be defined to realpath.
Perhaps this could be done conditionally on __DJGPP__ & __DJGPP_MINOR__, to
allow the same code to be used in the DXE3 project.
* Why does the code use _chmod in the ACCESS macro? DJGPP's access is
symlink-aware now, so perhaps that should be used in the ACCESS macro instead.
* FILENAME_MAX includes space for a terminating null. So fname (and some
others) doesn't need to be FILENAME_MAX + 1 big.
* The code doesn't cope with malloc failing.
src/libc/dxe/dlerror.c:
* _dl_unresolved_symbol is defined as 128 bytes in size in dlopen.c. The
buffer in dlerror is not big enough to hold a symbol name of that size.
Perhaps there should be a maximum symbol name length defined somewhere that
both files could use?
src/libc/dxe/dlopen.txh:
* It would be nice to have @ref{} or @xref{} or @pxref{} the functions listed
in @code{}, e.g.: dlclose.
* Please add a @vindex entry for LD_LIBRARY_PATH.
* How about a @ref{} or @xref{} or etc. to dxe3gen?
* NULL should be in @code{}, I think.
* "void *handle" isn't very texinfo. How about just "handle" instead?
* Again, @code{somefunction} doesn't need brackets after somefunction, when
you are just naming the function.
* "For DJGPP/COFF prepend and underscore in front.": "prepend and" -> "prepend
an".
* FWIW dlopen and dlclose are in the new POSIX standard.
src/libc/dxe/dlerror.txh:
* NULL should be in @code{}, I think.
* FWIW dlerror is in the new POSIX standard.
src/libc/dxe/dlregsym.txh:
* Again, @code{somefunction} doesn't need brackets after somefunction, when
you are just naming the function.
* "If no one is found, the last-resort handler is called." - no one -> none.
* Some periods (full-stops) don't have two spaces after them.
* "This is a *pointer* to a function" - pointer -> @emph{pointer} or perhaps
just remove the asterisks.
* "this allows to load modules which you don't know in advance which
unresolved symbols it contains" - "modules which you" -> "modules where you".
* "that is being called when" -> "that is called when". There is one case
where it says "when during" - I think the when needs to go just before "the
dynamic loader" in that case.
I'm sorry, but I didn't really get a chance to review the code of dxe3gen.c or
dxe3res.c or the tests. I also did not check what the DXE
loading/unloading/etc. code actually does on a functional level.
Some other comments:
Why not call the import libraries libsomething.ia instead of libsomething_i.a?
.ia works for long & short filenames.
It sounds like we should make a DXE version of libc as soon as possible, to
allow people to use libc functions in their DXEs without all the horrible
problems.
I wonder how hard it would be to come up with some support for DXEs in GNU
libtool...
Anyway, I don't think any of the above comments should stop the code going in
DJGPP 2.04 alpha 1. It would be nice if things were fixed after the alpha is
released, though...
Finally: Thanks!
Bye, Rich =]
--
Richard Dawe [ http://www.phekda.freeserve.co.uk/richdawe/ ]
- Raw text -