On Sun, 2009-05-10 at 16:35 +0100, Alan Jenkins wrote: > On 5/10/09, Andreas Robinson <andr345@xxxxxxxxx> wrote: > > Hi guys, > > > > following Robby's suggestion, I have moved to github. > > > > Please have a look at > > http://github.com/andr345/module-init-tools/commits/master/ > > > > (or git://github.com/andr345/module-init-tools.git) > > > > This is the second set of patches in the ELF-rewrite, organized for easy > > review. > > 1) I don't think load_strings() needs to be exported in struct > module_ops. It's easy enough to open-code and it assumes you need the > results in a stringtbl. It looks like it's just an implementation > detail of load_symbols(). Just like deref_sym() is an implementation > detail of fetch_tables(). I see your point. load_strings() might be used in a bunch of places in the next patchset, but I'll remove the export until it makes sense to put it back. > 2) "stringtbl" looks a little out of place here. > And I'm uneasy about > the way it's used for constant strings (weak ? "W" : "U") in > load_dep_syms(), Hmm ok, I will declare them outside the function somewhere. GCC treats constant strings as read-only data so they simply cannot go out of scope, but the C standard is unclear here. It might be implementation-specific. > as well as for malloc()'d strings. That the strings are malloc'd elsewhere should not be a problem if the caller is aware that the strings are in the ELF-file and that they remain available until release_file() is called. Depmod can't release any of the modules until it is finished. > I think a linked list with a field for the "weak" value would fit in > better. Allocate the string inline, like "struct symbol", and readers > don't have to worry about the string allocation as a separate issue. > You could call it "struct elf_symbol" to distinguish it. AFAIK, depmod is called from init in Debian's/Ubuntu's initramfs. My concern was that load_dep_syms() (and consequently depmod) would be slower if all the symbols in all modules were malloc'd and copied one by one. The string table is faster as there's no copying and fewer allocations. > 3) I tried running depmod under valgrind (you have to restrict the > number of modules, i.e. just copy a few modules to a staging directory > and use the -b option). Previously, depmod was valgrind-clean. > > ==12690== Conditional jump or move depends on uninitialised value(s) > ==12690== at 0x804A095: output_deps_bin (depmod.c:473) > ==12690== by 0x804CD85: main (depmod.c:1364) > ==12690== Uninitialised value was created by a stack allocation > ==12690== at 0x80517D3: add_value (index.c:86) > ==12690== > ==12690== Conditional jump or move depends on uninitialised value(s) > ==12690== at 0x804B066: output_symbols_bin (depmod.c:783) > ==12690== by 0x804CD85: main (depmod.c:1364) > ==12690== Uninitialised value was created by a stack allocation > ==12690== at 0x80517D3: add_value (index.c:86) > ==12690== > ==12690== ERROR SUMMARY: 4 errors from 2 contexts (suppressed: 11 from 1) > ==12690== malloc/free: in use at exit: 12,849 bytes in 138 blocks. > ==12690== malloc/free: 1,191 allocs, 1,053 frees, 321,180 bytes allocated. > > Did you try "tests/runtests --valgrind" ? Hopefully that should pick > it up as well, but it's a bit slow :-). How curious! I'll look into it. However, I tried valgrind before posting actually, and something is broken on my system and I haven't figured out what the problem is yet: $ sudo valgrind ./depmod ==32422== Memcheck, a memory error detector. [...snip banners...] ==32422== --32422:0:aspacem Valgrind: FATAL: VG_N_SEGNAMES is too low. --32422:0:aspacem Increase it and rebuild. Exiting now. I'm not sure why this happens. This is valgrind-3.4.1-Debian. Oh, and could you please send me add --valgrind patch please? I wasn't subscribed to this list when you posted it and the archive has mangled it somehow. (Or PEBKAC, more likely.) Cheers, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-modules" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
