This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: Create a hook for inspecting program headers during library load


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]