numatop: %{optflags} fail the 32bit build

[Dridi Boukelmoune]

The issue has been solved by my reviewer, so thank you all because as
usual I've learned interesting things :)

Comments inlined.

On Thu, Sep 12, 2013 at 2:53 PM, Florian Weimer <fweimer at redhat.com> wrote:
> On 09/12/2013 02:11 PM, Dridi Boukelmoune wrote:
>
>>> This version should work in 32 bit mode, and only in 32 bit mode:
>>>
>>> void
>>> cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
>>>        unsigned int *edx)
>>> {
>>>    __asm volatile
>>>      ("push %%ebx\n\t"
>>>       "cpuid\n\t"
>>>       "mov %%ebx, (%1)\n\t"
>>>       "pop %%ebx"
>>>       : "=a" (*eax),
>>>         "=S" (ebx),
>>>         "=c" (*ecx),
>>>         "=d" (*edx)
>>>       : "0" (*eax));
>>> }
>>
>>
>> I "kind of" understand what you're doing here, but it's not all clear.
>>
>> I get the push/pop instructions save and restore the reserved ebx
>> register, which is needed because apparently the cpuid instruction
>> would otherwise overwrite it.
>>
>> I don't understand the mov instruction, but I suppose you're storing
>> ebx's value from cpuid "somewhere else" before restoring it with the
>> pop instruction.
>
>
> Correct, the intent is to write the %ebx register value to the address in
> the %esi register.  "(%1)" is a pointer dereference, as oppose to plain %1.
>
>
>> I don't understand the last 5 lines of __asm in both functions, I've
>> never seen this syntax before.
>
>
> These are register constraints.  "a", "c", "d", "S" refer to the %eax, %ecx,
> %edx, %esi registers, respectively.  "=" marks output constraints.  The
> constraints before the final ":" are output registers, and after colon,
> there are the input registers.  There's just one, and "0" means to reuse the
> first output register.
>
> Okay, silly me, I should have listed "S" among the output registers, instead
> the inputs:

Actually, this morning (in the train) I've tested your code with my
tmp variable and it works if I remove the deref!

mov %%ebx, %1
(btw, what do %1 and %4 mean ?)

I could painlessly add a println in my patch since the file already
includes stdio.h. Normal and hardened build provide the same cpuid
values.

> void
> cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
>       unsigned int *edx)
> {
>   __asm volatile
>     ("push %%ebx\n\t"
>      "cpuid\n\t"
>      "mov %%ebx, (%4)\n\t"
>      "pop %%ebx"
>      : "=a" (*eax),
>        "=c" (*ecx),
>        "=d" (*edx)
>      : "0" (*eax),
>        "S" (ebx));
> }
>
> I also forget that for full correctness, there should now be a "memory"
> clobber as well (in the clobber section after yet another colon):

What would it do ? A compiler memory barrier ?

> void
> cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
>       unsigned int *edx)
> {
>   __asm volatile
>     ("push %%ebx\n\t"
>      "cpuid\n\t"
>      "mov %%ebx, (%4)\n\t"
>      "pop %%ebx"
>      : "=a" (*eax),
>        "=c" (*ecx),
>        "=d" (*edx)
>      : "0" (*eax),
>        "S" (ebx)
>      : "memory");
> }

It's a good thing my reviewer submitted a patch, because I wouldn't
feel that confident with mine :)

> By the way, we could generate much better code if the registers were passed
> as an array or struct, so that they are in consecutive memory:
>
> struct regs {
>   unsigned eax, ebx, ecx, edx;
> };
>
> void
> cpuid(struct regs *r)
>
> {
>   __asm volatile
>     ("push %%ebx\n\t"
>      "cpuid\n\t"
>      "mov %%eax, (%0)\n\t"
>      "mov %%ebx, 4(%0)\n\t"
>      "mov %%ecx, 8(%0)\n\t"
>      "mov %%edx, 12(%0)\n\t"
>      "pop %%ebx"
>      :
>      : "S" (r)
>      : "eax", "ecx", "edx", "memory");
> }
>
> Obviously, this needs adjustments to the callers.

Yup, but for the sake of simplicity, I wouldn't do that.

> --
> Florian Weimer / Red Hat Product Security Team

Dridi