Re: Object File Loader

[Andrew Lunn <andrew at lunn dot ch>]

> +cdl_package CYGPKG_OBJLOADER {
> +    display       "Object file loader"
> +    description   "This package provides support for loading and relocating 
> +                   object files withing eCos."
> +    include_dir   cyg/objloader
> +    include_files elf.h objelf.h loader_fs.h relocate_ppc.h
> +    compile       objloader.c objelf.c loader_fs.c
> +    requires      CYGPKG_MEMALLOC
> +    requires      CYGPKG_IO_FILEIO
> +# ====================================================================
> +
> +    cdl_component CYGPKG_SERVICES_OBJLOADER_ARCHITECTURE {
> +        display "Architecture dependent settings"
> +        flavor  none
> +        no_define
> +        description   "."

A description would be nice. 

> +
> +        cdl_option CYGBLD_OBJLOADER_ARCHITECTURE_POWERPC {
> +           display       "Support loading on PowerPC processors"
> +           calculated    CYGPKG_HAL_POWERPC
> +           define_proc {
> +               puts $::cdl_header "#include <cyg/objloader/relocate_ppc.h>"
> +           }
> +           compile relocate_ppc.c
> +        }
> + 
> +#        cdl_option CYGBLD_OBJLOADER_ARCHITECTURE_ARM {
> +#            display       "Support loading on ARM processors"
> +#            calculated    CYGPKG_HAL_ARM
> +#            define_proc {
> +#               puts $::cdl_header "#include <cyg/objloader/relocate_arm.h>"
> +#            }
> +#            compile relocate_arm.c
> +#        }
> +#    }

Would this be better if it was an interface. I expect the architecture
files will end up in the individial HALs, and it then implements the
interface. You can then also remove the define_proc since the HAL
would provide a <cyg/hal/relocate.h>

> +        cdl_option CYGPKG_SERVICES_OBJLOADER_CFLAGS_ADD {
> +            display "Additional compiler flags"
> +            flavor  data
> +            no_define
> +            default_value { "" }
> +            description   "
> +                This option modifies the set of compiler flags for
> +                building the serial device drivers. These flags are used in addition
> +                to the set of global flags."
> +        }

Line wrapping at 76 charactors would be nice.

The cdl to build the test case is missing.

> +2005-05-10  Anthony Tonizzo  <atonizzo@lycos.com>
> +
> +	* include/elf.h: 
> +	* include/loader_fs.h: 
> +	* include/objelf.h: 
> +	* include/relocate_ppc.h: 
> +	* src/loader_fs.c: 
> +	* src/objelf.c: 
> +	* src/objloader.c: 
> +	* src/relocate_ppc.c: 
> +	* doc/notes.txt: 
> +	* cdl/objloader.cdl: 
> +	Created OBJLDR package.

The documentation mentions a test case. It appears to be missing from
here but is included in the patch.

> +--------------------------------------------------------------------------------
> +Init/fini functions
> +--------------------------------------------------------------------------------
> +When the library is loaded the loader will look for a symbol with the name of 
> +"library_open" and, if found, will call the symbol with a prototype of
> +
> +void library_open( void )
> +
> +When the cyg_ldr_close_library() function is called, the loader will also 
> +look for a function called library close() and call it. The prototype is the 

library_close() - missing _ ?

Should these have cyg_ldr_ prefixes to avoid name space problems?

> +CYG_LDR_TABLE_ENTRY( diag_printf_entry, "diag_printf", diag_printf );
> +
> +The first parameter is a unique identifier, the second is a string containing
> +the name of the function, and the third is the pointer to the function itself.
> +The entries can be added anywhere in the file.

Is C++ name mangling supported in any way? I presume you would have to
give the mangled name here? Does this even make sense with C++?

> +The library can be compiled with the command
> +
> +gcc -c -Wall -o hello.o hello.c

XXX-elf-gcc

otherwise somebody will try to compile it with the host toolchain and
wonder why it does not work.... Also should there be an include path?

> +#ifndef __OBJELF_H__
> +#define __OBJELF_H__
> +
> +/* =================================================================
> + *
> + *      objelf.f

.h not .f

> +#ifndef __RELOCATE_PPC_H__
> +#define __RELOCATE_PPC_H__
> +
> +/* =================================================================
> + *
> + *      relocate_ppc.f

.h again

> +__inline__ size_t 
> +cyg_ldr_fs_read( PELF_OBJECT p, size_t s, size_t n, void *mem )
> +{
> +    return fread( mem, s, n, (FILE*)p->ptr );
> +}
> +
> +__inline__ cyg_int32 
> +cyg_ldr_fs_seek( PELF_OBJECT p, cyg_uint32 offs )
> +{
> +    return fseek( (FILE*)p->ptr, offs, SEEK_SET );
> +}
> +
> +__inline__ cyg_int32 
> +cyg_ldr_fs_close( PELF_OBJECT p )
> +{
> +    return fclose( (FILE*)p->ptr );
> +}
> +

Snip

> +    // Handlers for the file system open.
> +    e_obj->read  = cyg_ldr_fs_read;
> +    e_obj->seek  = cyg_ldr_fs_seek;
> +    e_obj->close = cyg_ldr_fs_close;

__inline__ makes no sense since you cannot inline into a structure.
They should be static.

> +void
> +*cyg_ldr_find_symbol( void* handle, cyg_uint8* sym_name )
> +{
> +    PELF_OBJECT p = (PELF_OBJECT)handle;
> +    cyg_uint8* tmp2;
> +    cyg_int32  i, symtab_entries;
> +    cyg_uint8 *p_strtab = (cyg_uint8*)p->sections[p->hdrndx_strtab];
> +    Elf32_Sym *p_symtab = (Elf32_Sym*)p->sections[p->hdrndx_symtab];
> +  
> +//    HAL_DCACHE_SYNC();

Can this be removed?

> +cyg_int32 
> +cyg_ldr_load_sections( PELF_OBJECT p )
> +{
> +    cyg_uint8  *error_string;
> +    cyg_int32  idx;
> +
> +    // Load the ELF header.
> +    p->p_elfhdr = (Elf32_Ehdr*)cyg_ldr_malloc( sizeof( Elf32_Ehdr ) );
> +    CYG_ASSERT( p->p_elfhdr != 0, "Cannot malloc() p->p_elfhdr" );
> +    if ( p->p_elfhdr == 0 )
> +        return -1;
> +    p->seek( p, 0 );
> +    p->read( p, sizeof( char ), sizeof( Elf32_Ehdr ), p->p_elfhdr  );
> +    error_string = cyg_ldr_sanity_check( p );
> +    if ( error_string != 0 )
> +    {
> +        cyg_ldr_last_error = "ERROR IN ELF HEADER";
> +        return -1;
> +    }    

Maybe a debug_printf of the error_string?

In general there appears to be lots of functions which are declared
globaly but should be static.

> +// -----------------------------------------------------------------------------
> +// External symbols.
> +// -----------------------------------------------------------------------------
> +
> +// eCos semaphores
> +CYG_LDR_TABLE_ENTRY( cyg_thread_delay_entry,
> +                     "cyg_thread_delay",   
> +                     cyg_thread_delay );
> +CYG_LDR_TABLE_ENTRY( cyg_semaphore_wait_entry,
> +                     "cyg_semaphore_wait", 
> +                     cyg_semaphore_wait );
> +CYG_LDR_TABLE_ENTRY( cyg_semaphore_init_entry,
> +                     "cyg_semaphore_init", 
> +                     cyg_semaphore_init );
> +CYG_LDR_TABLE_ENTRY( cyg_semaphore_post_entry,
> +                     "cyg_semaphore_post", 
> +                     cyg_semaphore_post );

How about putting a variation of this into the main src directory. 
Have CDL control each group of symbols:

cdl_componet CYGPKG_LDR_EXPORT {
   description "Enable the export of KAPI functions"

   cdl_component CYGPKG_LDR_EXPORT_KAPI {
        description "Export the KAPI functions "
        cdl_option CYDBLD_LDR_EXPORT_KAPI_SEMAPHORE {
        }

        cdl_option CYDBLD_LDR_EXPORT_KAPI_MAILBOX {
        }
   }
   cdl_component CYGPKG_LDR_EXPORT_LIBC {
    etc...

If this is not fine grained enough the programmer can do his own, but
i expect for most cases this will be a good starting point.

        Andrew