Re: [patch] clear direction flag for signal handlers

[Aggelos Economopoulos <aoiko@xxxxxxxxxxxxxx>]

On Tuesday 11 March 2008, Matthew Dillon wrote:
> 
> :Aggelos Economopoulos wrote:
> :> BTW, any opinions on how we could reduce the code duplication between pc32/*
> :> and vkernel/i386 (and maybe pc64/amd64) piecemeal?
> 
>     There actually isn't a whole lot of commonality.  What we MUST avoid is
>     trying to have a single piece of code with a lot of #ifdef's in it.  We also
>     really have to avoid to much cross-wiring, where code in one platform's Makefile
>     directly references code in another.  It's a real mistake to do that because
>     it means making changes in one architecture will wind up having a huge and
>     probably unintended effect on the others.  It's all about code maintainance.

Well, there is still *some* identical code, or pieces of code that could
easily be shared (see below). Nobody suggested using #ifdefs. One idea
would be to have a file with the shareable code and pc32/vkernel would
each have a file with the functions they implement differently. If a function
only differs in some code block and that block can be cleanly turned into a
function, do just that. Or, for example in sendsig() define a macro
is_vm86(regs) (or whatever), only define it as 0 in the appropriate vkernel
include and you're done. The compiler will just throw away the dead code, no
#ifdefs involved. Etc.

As for changes in one arch (assuming you mean platform here) breaking
another, well, although it's a real possibility I don't think it is
very probable. If you're making big changes, you'll normally end up updating
the vkernel side as well, so having a mostly-common codebase forces you to
think about the vkernel too from the start. Sorry, but I can't give a more
concrete answer to this hypothetic scenario.

Unfortunately this isn't very convincing, so I'll just claim that the
benefits from any code sharing outweigh any possibilities for accidental
breakage. When you fix a bug (or clean it up, or even add a feature) in
duplicated code you must remember to propagate this change to all duplicates.
I think most people will agree that relying on the programmer keep track of
duplicates is a recipe for lost bugfixes and diverging code copies.

>     When I created vkernel I put a ton of common code in the cpu/ infrastructure,
>     and moved other more machine-independant bits out of platform/pc32 and into the
>     mainline kernel itself.  I didn't separate absolutely everything but I got enough
>     of it.

Using a tool for finding similar code for guidance, I glanced at similar
files in the pc32 and vkernel directories. This is only a hasty first look,
but here's is what I found. IMHO, there is some potential for code sharing
and it is worth the effort to gradually try bringing the trees closer. That
said, I'm not very familiar with the build system so I'm not sure how easy
it would be to do the necessary changes and I can't volunteer to do all the
work myself :/

platform/{vkernel,pc32}/i386/procfs_machdep.c
are identical

platform/{vkernel,pc32}/i386/tls.c
differ by one stub function and 1 include line

platform/{pc32/isa,vkernel/platform}/ipl_funcs.c
differ in 3 include lines and one extra comment

platform/{pc32/i386,vkernel/platform}/busdma_machdep.c
the diffstat for those is
 busdma_machdep.c |   75 +++++++++++--------------------------------------------
 1 file changed, 15 insertions(+), 60 deletions(-)
and i386/busdma_machdep.c is 941 lines long
The differences are an early panic for the vkernel version in
free_bounce_page() an extra function and a block that is only needed in the
pc32 case. Seems mergeable, but >80% of the code can be shared right now.

platform/{pc32,vkernel}/i386/db_trace.c
are similar, but not too promising.

platform/{pc32,vkernel}/i386/genassym.c
IMHO beg to have their common lines split out.

platform/{pc32/i386/machdep,vkernel/i386/cpu_regs}.c
Lot's of functions that are pc32-only, vm86 differences seem easy to merge, a
symbol difference (Maxmem vs physmem), sys_sigreturn() needs some thought.

platform/{pc32,vkernel}/i386/vm_machdep.c
extra casts in vkernel(?), some missing code, kvm_access_check() mergeable.

platform/{pc32,vkernel}/i386/autoconf.c
can share some lines

platform/{pc32,vkernel}/i386/db_interface.c
Debugger()

platform/{pc32/isa,vkernel/i386}/npx.c
would need some work.

platform/{pc32,vkernel}/i386/trap.c
a few functions are identical, trap()/user_trap() which is ~1/3 of the
file is not easy to merge.

pmap.c is significantly different (not surprisingly)

Aggelos