This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
RE: Create a hook for inspecting program headers during library load
- From: Matthew Fortune <Matthew dot Fortune at imgtec dot com>
- To: Roland McGrath <roland at hack dot frob dot com>
- Cc: "libc-alpha at sourceware dot org" <libc-alpha at sourceware dot org>
- Date: Wed, 1 Oct 2014 22:21:26 +0000
- Subject: RE: Create a hook for inspecting program headers during library load
- Authentication-results: sourceware.org; auth=none
- References: <6D39441BF12EF246A7ABCE6654B0235320F18A1B at LEMAIL01 dot le dot imgtec dot org> <20141001214949 dot 239022C3AAD at topped-with-meat dot com>
Roland McGrath <roland@hack.frob.com> writes:
> > static inline bool
> > elf_machine_phdr_check (const ElfW(Phdr) *phdr, ElfW(Half) phnum,
> > const char *buf, ssize_t len, int fd,
> > struct link_map *map)
>
> Never use ssize_t in a case like this. It is only for return values.
> Use size_t here. Use uint_fast16_t in place of ElfW(Half).
> The indentation of the later parameter lines is wrong.
Indeed, just copy-paste badness, the patch is correct.
> Avoid an ambiguous name like "..._check". It's better to pick a name
> where the sense of the return value is unambiguous, by using a verb in
> the name or using the "..._adjective_p" convention.
>
> This has just one caller and that caller is a simple if statement, so it
> is cleaner if the sense of the function's value is in the direction the
> if test wants. IOW, "if (__glibc_unlikely (...))" always looks better
> than "if (!__glibc_likely (...))" (and avoids the discussion about
> whether "if (__glibc_unlikely (!...))" is preferable).
Seems reasonable, I chose that style to follow this existing code though:
if (! __glibc_likely (elf_machine_matches_host (ehdr)))
goto close_and_out;
I'm struggling to think of a good way to invert the sense of the name:
elf_machine_phdr_not_ok_p - still has a not in it.
elf_machine_phdr_bad_p - doesn't really say the right thing
elf_machine_phdr_incompatible_p - long winded but accurate
> > What sort of testing is needed to commit this (I have only done
> > x86_64-linux-gnu and mips*-linux-gnu)?
>
> Ideally build-testing of the other machines you've touched. But it's not
> really necessary in a boilerplate case like this.
>
> > I am happy to put the outcome of this thread on the wiki.
>
> I'm not sure what addition you have in mind that would be useful.
There have been various threads edging in this direction and the last one
asked if I could add some description of how MIPS loadable ABI information
(beyond the ELF flags) was generally going to work. I'm not entirely sure
what I'll put either but a basic flow of how information gets from compiler
through to ld.so seems appropriate.
> Please avoid using attachments if you possibly can. We really want just
> plain text patches at the end of the plain text message. (If you really
> can't, then consider getting a MUA that doesn't constrain your composition
> choices unreasonably.)
OK. I went for an attachment as the patch sprawled somewhat. I have a few
thousand line patch coming next (sorry :-)), do you want everything inline
unless it exceeds the message limit?
> The approach you've taken is probably fine. (However, it does concern me
> that you added boilerplate to all the dl-machine.h files and made exactly
> one nonidentical: the mips one is "static bool __attribute_used__" for no
> apparent reason while the others are all "static inline bool".) But when
The patch is split out from my overall work and I had that in the actual
implementation of the function for MIPS (for no good reason).
> you have to repeat a boilerplate definition a bunch of times, and
> especially when it seems likely that most of those places will never
> change, then it should be a red flag. An alternative approach is to add
> a new sysdeps header file just for the new purpose. Then you not only do
> not have to repeat the boilerplate (because you just write one
> sysdeps/generic/--or in this case, perhaps just elf/--stub version that
> everything uses), but you touch only "generic" code and so even the pro
> forma ideal testing requirements are less.
That sounds ideal but I'm not 100% sure what you mean. I am assuming a
new header would be #included into all the dl-machine.h files?
Thanks for the quick feedback.
Matthew