loadelf_32.c: code review request: was=>Re: ia64 unwind section: Loader implementation

[Cherry G. Mathew-2]

On 12/31/05, Cherry George Mathew <cherry@...> wrote:

> On Fri, 2005-12-30 at 09:38 -0500, Allen Briggs wrote:
>
> > Aside from that, this seems like the best option to me, but can it
> > be abstracted a little bit into an md callback?  Either to allow
> > machine-specific selection of extra sections to load (I think this
> > is my preference without digging into the code) or to just call a
> > machine-specific routine to load what it needs?
> >
>
> On IA64, I found it best to implement it via a simple macro which
> returns boolean about whether the
> segment needs to be loaded. The macro would be passed the whole Phdr
> structure which would enable
> the md hook to do post processing ( reloc type thingies ? ) if
> required.
>
> For IA64 unwind sections, a simple one liner did the trick:
>
> #define MD_LOADSEG(phdr) (phdr.p_type == PT_IA_64_UNWIND ? TRUE : FALSE)
>
> I've filed a PR: kern/32423
> http://netbsd.org/cgi-bin/query-pr-single.pl?number=32423
>
> Thanks,
>
> Cherry
>
>

Hi,

Would this affect other ports ?

Thanks,

Cherry


cvs diff -uN loadfile_elf32.c
Index: loadfile_elf32.c
===================================================================
RCS file: /cvsroot/src/sys/lib/libsa/loadfile_elf32.c,v
retrieving revision 1.13
diff -u -r1.13 loadfile_elf32.c
--- loadfile_elf32.c 25 Jan 2006 18:27:23 -0000 1.13
+++ loadfile_elf32.c 1 Apr 2006 19:02:52 -0000
@@ -296,6 +296,13 @@

  for (first = 1, i = 0; i < elf->e_phnum; i++) {
  internalize_phdr(elf->e_ident[EI_DATA], &phdr[i]);
+
+#ifdef MD_LOADSEG /* Allow processor ABI specific segment loads */
+ if ( (phdr[i].p_type & PT_LOPROC) &&
+     MD_LOADSEG(phdr[i]))
+ goto loadseg;
+#endif /*MD_LOADSEG*/
+
  if (phdr[i].p_type != PT_LOAD ||
     (phdr[i].p_flags & (PF_W|PF_X)) == 0)
  continue;
@@ -309,6 +316,7 @@
  if ((IS_TEXT(phdr[i]) && (flags & LOAD_TEXT)) ||
     (IS_DATA(phdr[i]) && (flags & LOAD_DATA))) {

+ loadseg:
  /* Read in segment. */
  PROGRESS(("%s%lu", first ? "" : "+",
     (u_long)phdr[i].p_filesz));


--
~Cherry

[Matt Thomas]

On Apr 1, 2006, at 11:13 AM, Cherry G. Mathew wrote:

> +     MD_LOADSEG(phdr[i]))

You should be passing &phdr[i] so that if MD_LOADSEG results in
a function call you are passing by reference.

[Cherry G. Mathew]

On 4/3/06, Christos Zoulas <christos@...> wrote:

> In article <A23ABE43-CFE0-476E-8663-991A757ABF7F@...>,
> Matt Thomas  <matt@...> wrote:
> >
> >On Apr 1, 2006, at 11:13 AM, Cherry G. Mathew wrote:
> >
> >> +                 MD_LOADSEG(phdr[i]))
> >
> >You should be passing &phdr[i] so that if MD_LOADSEG results in
> >a function call you are passing by reference.
>
> I've been thinking exactly the same thing :-)
>
> christos
>
>

Sounds reasonable to me.
Lets hold on to this for the moment for more comments, if any.

--
~Cherry

[Jason Thorpe]

On Apr 1, 2006, at 11:13 AM, Cherry G. Mathew wrote:

> cvs diff -uN loadfile_elf32.c
> Index: loadfile_elf32.c
> ===================================================================
> RCS file: /cvsroot/src/sys/lib/libsa/loadfile_elf32.c,v
> retrieving revision 1.13
> diff -u -r1.13 loadfile_elf32.c
> --- loadfile_elf32.c 25 Jan 2006 18:27:23 -0000 1.13
> +++ loadfile_elf32.c 1 Apr 2006 19:02:52 -0000
> @@ -296,6 +296,13 @@
>
>   for (first = 1, i = 0; i < elf->e_phnum; i++) {
>   internalize_phdr(elf->e_ident[EI_DATA], &phdr[i]);
> +
> +#ifdef MD_LOADSEG /* Allow processor ABI specific segment loads */
> + if ( (phdr[i].p_type & PT_LOPROC) &&
> +     MD_LOADSEG(phdr[i]))
> + goto loadseg;
> +#endif /*MD_LOADSEG*/

Does't it make sense to do >= LOPROC && <= HIPROC instead of testing  
a bit?

> +
>   if (phdr[i].p_type != PT_LOAD ||
>      (phdr[i].p_flags & (PF_W|PF_X)) == 0)
>   continue;
> @@ -309,6 +316,7 @@
>   if ((IS_TEXT(phdr[i]) && (flags & LOAD_TEXT)) ||
>      (IS_DATA(phdr[i]) && (flags & LOAD_DATA))) {
>
> + loadseg:
>   /* Read in segment. */
>   PROGRESS(("%s%lu", first ? "" : "+",
>      (u_long)phdr[i].p_filesz));
>
>
> --
> ~Cherry

-- thorpej