Subject: Re: loadelf_32.c: code review request: was=>Re: ia64 unwind section: Loader implementation
To: None <tech-kern@netbsd.org>
From: Christos Zoulas <christos@astron.com>
List: tech-kern
Date: 04/01/2006 19:50:05
In article <9b7438470604011113n70f57d52x9a14d28b9871883b@mail.gmail.com>,
Cherry G. Mathew <cherry@sdf.lonestar.org> wrote:
>On 12/31/05, Cherry George Mathew
><cherry@sdf.lonestar.org> 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*/

I would just include the test for PT_LOPROC in MD_LOADSEG... like:

#ifndef MD_LOADSEG
#define MD_LOADSEG(a)	/*CONSTCOND*/0
#endif
		if (MD_LOADSEG(phdr[i]))
			goto loadseg;

So that I don't get the warning about the label not used or litter
the code with even more ifdefs.

christos

>+
> 		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
>