DragonFlyBSD

You might want to pull this up to vkernel as well.

Else, looks good.

cheers
simon

And to amd64 as well, otherwise this might stay unnoticed for a long time.
Wait. Did I even fix regular i386? Wouldn't be surprised if I've sent an
empty patch...

BTW, any opinions on how we could reduce the code duplication between pc32/*
and vkernel/i386 (and maybe pc64/amd64) piecemeal?

Index: platform/pc32/i386/machdep.c ===================================================================
retrieving revision 1.129
diff u -r1.129 machdep.c
-- platform/pc32/i386/machdep.c
+++ platform/pc32/i386/machdep.c
@ -515,7 +515,13 @ ===================================================================
retrieving revision 1.4
diff u -r1.4 cpu_regs.c
-- platform/pc64/amd64/cpu_regs.c
+++ platform/pc64/amd64/cpu_regs.c
@ -327,7 +327,13 @ ===================================================================
retrieving revision 1.24
diff u -r1.24 cpu_regs.c
-- platform/vkernel/i386/cpu_regs.c
+++ platform/vkernel/i386/cpu_regs.c
@ -325,7 +325,13 @

regs->tf_esp = (int)sfp;
     regs->tf_eip = PS_STRINGS - ;
-    regs->tf_eflags &= ~PSL_T;

    /
+     * i386 abi specifies that the direction flag must be cleared
+     * on function entry
+     */
+    regs->tf_eflags &= ~(PSL_T|PSL_D);
+
     regs->tf_cs = _ucodesel;
     regs->tf_ds = _udatasel;
     regs->tf_es = _udatasel;

Introduce platform/pc/ where all the code is kept, including a
platform/pc/conf/files, hooking it into the builds much like how cpu/
and platform/<platform>/ are. Don't introduce a new dir keyword for
includes, but just overload on machine/ and machine_base like e.g.
cpu/i386/include already is now.
--
Thomas E. Spanjaard
tgen@netphreax.net

: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.

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.

-Matt

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.

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

: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.

Heh.  No way.  A macro used for conditional compilation is really
    no different from an #ifdef.  We're definitely not going to do
    that.

: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.

It's not only probable, it's virtually guaranteed to occur.  Trying to
    share code through even moderate conditionalization is a big mistake
    from the point of view of then having to maintain that code.  Sure,
    some things you do want to try to share... if we had 50 different
    platforms all sharing the same bit of code then it would make sense
    to put that code in a machine independant directory.  But the last 
    thing we want to do is have excessive cross-platform code sharing.

: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.

It's no better the other way.  When you make a change in a piece of
    common code, you have no guarantee that your change will not break
    platforms other then the one you tested on.  This is particularly true
    of any bit of code with #ifdef's (or conditional compilation macros).

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

This is almost machine-independant code already, it could probably
    be moved to the cpu-specific directory (/usr/src/sys/cpu/i386).

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

This could probably be moved too.

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

This is getting a bit more iffy.  The IPL functions are liable to
    change in the future in ways that will diverge.

: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.

None of these are good candidates, not even genassym.c.  As the code
    evolves changes made to these files are almost guaraneteed to diverge.

: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.

None of these are good candidates, for the same reason.  The NPX code
    is a good example of why it isn't a good idea.  When I originally broke
    the vkernel off I spent several hours trying to share the npx code.
    It was a dismal failure.  Despite the fact that nearly every change
    to npx we make to pc32 has to be reflected in vkernel, I still far
    prefer them to be separate.

:pmap.c is significantly different (not surprisingly)
:
:Aggelos

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

But it is different.

compare

if (has_vm(regs)) {
code;
}

with

#ifndef _KERNEL_VIRTUAL
if (regs->tf_eflags & PSL_VM) {
code;
}
#endif

In the first case you have to parse one conditional, the same conditional that
was there before vkernel was added to the tree. The macro just serves as an
optimal _predict_false(regs->tf_eflags & PSL_VM) that effectively does
_assume_false() and allows the compiler to remove the code at compile time.
There is no extra logic.

The second case adds an extra conditional that is evaluated at a different time
and uses different syntax than the rest of the code. Repeat a couple of times
(with #else too), intersperse some C conditionals and you end up with the kind
of mess that rightly gave #ifdef its bad name.

I suppose your argument here, like mine, is based on anecdotal evidence. So
I guess a reasonable answer would be "No, no, no, you're wrong, I'm right" ;)

It seems to me that's just as true when you change code that is shared by
not just two, but by all platforms (i.e. generic code). I don't think
things break that often[*].

Since I don't think I can convince you to change your mind, I'll be polite
and not waste your time (and mine) by arguing the remaining points :)

Aggelos

[*] Unless you force code to be shared where it shouldn't but that's not
what I suggested.

swildner@ pointed out that there's yet another path that ends up running
a signal handler (for linux binaries). Updated patch follows.

gcc-4.3 assumes the direction flag is clear on function entry as
specified by the i386 abi. Ensure that is the case when running
a signal handler.

Linux-kernel discussion with gcc people starts here:
http://article.gmane.org/gmane.linux.kernel/650279

Index: platform/pc32/i386/machdep.c ===================================================================
retrieving revision 1.129
diff u -r1.129 machdep.c
-- platform/pc32/i386/machdep.c
+++ platform/pc32/i386/machdep.c
@ -515,7 +515,13 @ ===================================================================
retrieving revision 1.24
diff u -r1.24 cpu_regs.c
-- platform/vkernel/i386/cpu_regs.c
+++ platform/vkernel/i386/cpu_regs.c
@ -325,7 +325,13 @ ===================================================================
retrieving revision 1.4
diff u -r1.4 cpu_regs.c
-- platform/pc64/amd64/cpu_regs.c
+++ platform/pc64/amd64/cpu_regs.c
@ -327,7 +327,13 @ ===================================================================
retrieving revision 1.29
diff u -r1.29 linux_sysvec.c
-- emulation/linux/i386/linux_sysvec.c
++ emulation/linux/i386/linux_sysvec.c
@ -369,7 +369,13 @
regs->tf_esp = (int)fp;
regs->tf_eip = PS_STRINGS - +
linux_sznonrtsigcode;
- regs->tf_eflags &= ~(PSL_T | PSL_VM);

+ /
+ * i386 abi specifies that the direction flag must be cleared
+ * on function entry
+ /
+ regs->tf_eflags &= ~(PSL_T | PSL_VM | PSL_D);

regs->tf_cs = _ucodesel;
regs->tf_ds = _udatasel;
regs->tf_es = _udatasel;
@ -503,7 +509,13 @
*/
regs->tf_esp = (int)fp;
regs->tf_eip = PS_STRINGS - *(p->p_sysent->sv_szsigcode);
- regs->tf_eflags &= ~(PSL_T | PSL_VM);

+ /
+ * i386 abi specifies that the direction flag must be cleared
+ * on function entry
+ */
+ regs->tf_eflags &= ~(PSL_T | PSL_VM | PSL_D);
+
regs->tf_cs = _ucodesel;
regs->tf_ds = _udatasel;
regs->tf_es = _udatasel;

regs->tf_rsp = (int)sfp;
     regs->tf_rip = PS_STRINGS - ;
-    regs->tf_rflags &= ~PSL_T;

    /
+     * amd64 abi specifies that the direction flag must be cleared
+     * on function entry
+     /
+    regs->tf_rflags &= ~(PSL_T|PSL_D);
+
     regs->tf_cs = _ucodesel;
     / regs->tf_ds = _udatasel;
     regs->tf_es = _udatasel; */
Index: emulation/linux/i386/linux_sysvec.c

:swildner@ pointed out that there's yet another path that ends up running
:a signal handler (for linux binaries). Updated patch follows.
:
:gcc-4.3 assumes the direction flag is clear on function entry as
:specified by the i386 abi. Ensure that is the case when running
:a signal handler.
:
:Linux-kernel discussion with gcc people starts here:
:http://article.gmane.org/gmane.linux.kernel/650279
:
:Index: platform/pc32/i386/machdep.c
:===================================================================
:retrieving revision 1.129
:diff u -r1.129 machdep.c
:-- platform/pc32/i386/machdep.c
:+++ platform/pc32/i386/machdep.c
:@ -515,7 +515,13 @
:
: regs->tf_esp = (int)sfp;
: regs->tf_eip = PS_STRINGS - ;
:- regs->tf_eflags &= ~PSL_T;
:+
:+ /
:...

It all looks good, definitely commit it and maybe even MFC it to
    the release branch too.

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

On Friday 14 March 2008, Matthew Dillon wrote:
[...]

It all looks good, definitely commit it and maybe even MFC it to
the release branch too.

Committed. We don't have gcc-4.3 or icc in 1.12 so I don't think this needs
to go in the release branch (but if it did, the correct way to do it would
be with "cvs commit -r DragonFly_RELEASE_1_12 [files]", right? Sorry, but
I haven't used cvs for development since the first free software distributed
scm became available[*], so perhaps there's an extra quirk I don't remember
about?)

Aggelos

[*] If you think git is weird, you should try pre-tla arch :)

If it's not too much trouble, could you clarify exactly what you don't like
with this concrete example? I think my explanation of why this is not
equivalent with an #ifdef is reasonable.

Thanks,
Aggelos

Please MFC it. The kernel violates the platform ABI and if anyone
wants to run such a binary, she will run into issues.

Joerg

:On Friday 14 March 2008, Matthew Dillon wrote:
:[...]
:> It all looks good, definitely commit it and maybe even MFC it to
:> the release branch too.
:
:Committed. We don't have gcc-4.3 or icc in 1.12 so I don't think this needs
:to go in the release branch (but if it did, the correct way to do it would
:be with "cvs commit -r DragonFly_RELEASE_1_12 [files]", right? Sorry, but
:I haven't used cvs for development since the first free software distributed
:scm became available[*], so perhaps there's an extra quirk I don't remember
:about?)
:
:Aggelos
:
:[*] If you think git is weird, you should try pre-tla arch :)

I wouldn't recommend using a cvs commit line like that, it's liable
    to create problems.

What I suggest is you do a checkout, in a different directory, of
    the 1.12 release branch.  Do the MFC's within that checkout and commit
    normally.  Most of the time you can patch changes from HEAD into
    a checked out release by using something like this:

cvs diff -r<rev1> -r<rev2> file | patch

It's more steps to do it this way, but much safer and you can review
    the changes made prior to committing them.

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>