Request for comments: many small commits

[Antoine LECA]

Hi guys,

I would like to receive comments about a number of small things I am
working with for several weeks, and that I believe are mature enough to
be used in the wild and perhaps committed to official MINIX.

The code can be browsed at

https://github.com/antoineL/minix/commits/WRK0408e

There are many small commits to ease review and also to ease the work to
merge them perhaps individually. I won't comment each commit, just
highlight that many are trying to get rid of old cruft as it exists in
every source tree after a while; several commits are trying to allow to
run a stable system with dynamic libraries for the utilities in
/usr/bin/* (services and the utilities in the root file system are kept
static; in other words I keep MKDYNAMICROOT=no)
Another class of commits are fixes for small glitches (like missing
dependencies), or improvements for cases which may not be used by
everybody (like using build.sh -M)
I also did a pass over the assembly code (after I spotted a problem with
libc's PIC code); something I removed entirely is the code in kernel's
klib.S for smp_get_* functions, which appeared unused and distinct from
their documentation.

If there are any obscure corner, I'll happy to help you understand what
my intent might have been.


Enjoy!

Antoine

[Ben Gras]

Replying to confirm & for visibility. I rebased your branch to master and for now cherry-picked your pic assembly change for testing, as that DT_TEXTREL in libc has been haunting me forever.

[Ben Gras]

Hi Antoine,

So for the PIC patch. I'd like to take it to make libc PIC. But I can't read the code :).

Is there any chance you could minimize the change (make it just for making it PIC) and give a little more explanation about what's happening?

Thanks!

On Tuesday, April 9, 2013 7:46:15 PM UTC+2, AntoineLeca wrote:

[Antoine LECA]

Ben Gras wrote:
> So for the PIC patch. I'd like to take it to make libc PIC. But I can't
> read the code :).
>
> Is there any chance you could minimize the change (make it just for making
> it PIC) and give a little more explanation about what's happening?

I guess you are referring to
https://github.com/antoineL/minix/commit/866c34e

I believe it was all about making the code PIC, even if sometimes it is
not just inserting _PIC_PLT in the call instructions.

I assume you are not worried by the 15 lines of obsolete ACK directives
removed at the top of lib/libc/arch/i386/sys-minix/ucontext.S

The stuff about sigreturn is IMHO clear and I hope correctly documented.
It also highlights the most evident changes one has to do to make sure
assembly code is PIC-compliant:
- use the macros PIC_PROLOGUE and PIC_EPILOGUE around any use of PIC'd
features, including any call to non-local code and use of shared
variables (with GOT or GOTOFF; not used here, and best avoided)
- use the macro PIC_PLT for any relative reference (call or jmp) to a
code label in another source file
- these macros are /implicitly/ using the %ebx register, which become
useless for the normal assembler code
- the previous value of %ebx is saved on stack, so should be accounted
for in any stack manipulation

Also important is to remember that numeric label 1: is also used
implicitly by PIC_PROLOGUE, so it is better to avoid it is assembly code
(I spend a fair time chasing that one.)

Another thing which should be clear is when one needs to update errno
(errno is *not* a single variable in libc.so); I copied code used in
other parts of NetBSD's libc, using the __errno() non-local function.

Ok; so here comes the real meat, explanations to my changes:

About the rest of getcontext stuff, I agree I went a bit farther since I
ended up in shortage of registers and I needed to streamline the code to
deal with stack depth variation; yet I believed that replacing
lea UC_FLAGS(%edx), %ebx /* ebx = &(ucp->uc_flags) */
mov (%ebx), %ecx /* ecx = ucp->uc_flags */
with
mov UC_FLAGS(%edx), %eax /* eax = ucp->uc_flags */
would be OK.
Then, I did replace
mov $UCF_IGNFPU, %eax
or $UCF_IGNSIGM, %eax
cmp %eax, %ecx /* is UCF_IGNFPU or UCF_IGNSIGM set? */
jz 1f /* Both are set, skip getuctx */
with
xor $[UCF_IGNFPU|UCF_IGNSIGM], %eax /* toggle both flags */
jz 5f /* Both were set, skip getuctx */
which does the same in fewer instructions (and avoiding the slightly
misleading comment at the cmp intruction.)
I inserted the and instruction to check only those two flags, which
seemed a sensible thing to do here; however I preserved the semantics
and kept it as #if 0 (in my tests, it worked OK when enabled; but I feel
there are no other flags in current use at the moment.)
Then I removed the already-done-above
mov 4(%esp), %edx /* edx = ucp */
Then I replaced
pop %eax /* retaddr */
mov %eax, PC(%edx) /* Save real RTA in mcp struct */
with
pop PC(%edx) /* Save real RTA in mcp struct */
And finally I dropped the whole use of %ecx as a temporary through the
whole function, and used PC(%edx) which holds the same value instead,
exactly the same as the code for setcontext() was doing; if there are
special debugging tricks which require managing those two copies in
separate ways, it was not obvious from the comments.


About setcontext stuff, they are quite the same kind of modifications:
lea MAGIC(%edx), %ebx /* ebx = &(ucp->mc_context.mc_magic) */
mov (%ebx), %ecx /* ecx = ucp->mc_context.mc_magic */
cmp $MCF_MAGIC, %ecx /*is the magic value set (is ctxt valid)?*/
is replaced with the equivalent
cmpl $MCF_MAGIC, MAGIC(%edx) /* is the magic value set //...
the check for the flags is the same trick as above, with the same added
and disabled code to deal only with those flags; the code to call
__errno() and set it is kept in only one place, which should be OK; I
changed a comment about the effect of `jmp *PC(%edx)` (the #if 0 there
is obviously a trace from past experimentations which can be dropped);
and finally there is the stuff at the end of the function which deals
with the saved-on-stack copy of %ebx, which I expect to be correctly
explained by the comments.

Feel free to ask for more explanations.

I have thought a bit today about making the change smaller, that is, to
avoid all the assembly tricks (xor, pop_to_memory, use of indexed mode)
which seem natural to me after severaal years of x86 assembly, but can
legitimately raise doubts; however, those "tricks" have the benefit to
avoid using more registers and in effect to push many variables on the
stack; furthermore, since use of %ebx is frowned on in PIC mode, it was
not obvious to me that the change will result significantly smaller; but
if you consider it has to be done taht way, I would consider reviewing
the code as soon as time would allow it.


Antoine

[Ben Gras]

Thank you for your detailed explanation. I do believe I can read it now.

On Tuesday, April 9, 2013 7:46:15 PM UTC+2, AntoineLeca wrote:

[Ben Gras]

Hi Antoine,

The PIC change is merged after light modification, thank you.