Re: [PATCH v2] grub-core: Build fixes for i386

[Jan Nieuwenhuizen]

Daniel Kiper writes:

Hello,

> Mostly nits... Please take a look below...

Great!

> On Tue, May 18, 2021 at 12:47:33PM +0200, Jan (janneke) Nieuwenhuizen wrote:
>> To reproduce, update the Grub source description in your local Guix
>
> s/Grub/GRUB/

Ok.

>> or install an x86 cross-build environment on x86-linux (32bit!) and
>
> s/32bit/32-bit/

Ok.

>> configure to cross build and make, e.g., do something like
>>
>>     ./configure \
>>        CC_FOR_BUILD=gcc \
>>        --build=i686-unknown-linux-gnu
>
> Missing "\" at the end of the line...

Fixed.

>> * grub-core/lib/i386/relocator64.S: Avoid x86_64 instructions on i386.
>
> Hmmm... What is this?

The "gitlog-to-changelog" scripts needs entries like this in order to
generate a GNU-compliant ChangeLog, see Emacs, LilyPond, Guile, Guix,
etc.  GRUB is the first GNU project that I encounter that has a
different take on this; sorry for missing that!

I have changed it to

--8<---------------cut here---------------start------------->8---
This fixes cross-compiling to x86 (e.g., the Hurd) from x86-linux of

    grub-core/lib/i386/relocator64.S

This file has six sections that only build with a 64-bit assembler,
yet only the first two sections had support for a 32-bit assembler;
this patch completes this for the remaining sections.
--8<---------------cut here---------------end--------------->8---

> And you should add your SOB at the end of commit message:
>
> Signed-off-by: Jan (janneke) Nieuwenhuizen <janneke@gnu.org>

Done.

And sorry; you asked that before.  I failed to do so because I only ever
encountered this use (e.g., quoting the Guix manual)

       When pushing a commit on behalf of somebody else, please add a
    ‘Signed-off-by’ line at the end of the commit log message—e.g., with
    ‘git am --signoff’.  This improves tracking of who did what.

and as I am the author, that would be redundant.

>> ---
>>  grub-core/lib/i386/relocator64.S | 27 +++++++++++++++++++++++++--
>>  1 file changed, 25 insertions(+), 2 deletions(-)
[..]
>> +    .byte 0x48
>> +        .byte 0x83
>> +        .byte 0xe4
>> +        .byte 0xf0
>
> Formatting is broken here...

Oops!  Apparently GRUB uses TAB characters.  Fixed!

(A .dir-locals.el file at the project root is often to avoid such
mistakes.)

>> +#ifdef      __x86_64__
>>      movq    %rax, %rsi
>> -
>
> This and...
>
>> +#else
>> +    /* movq %rax, %rsi */
>> +    .byte   0x48
>> +    .byte   0x89
>> +    .byte   0xc6
>> +#endif
>> +
>
> ... this change look strange. Could you fix it? Or explain in the
> commit message you are doing a cleanup by the way...

Hmm...what is it that looks strange here?  Obviously the MOV %RAX..
statement must be guarded and get a 32-bit alternative, and the empty
line after the move instruction now moves to the end of the block?

Greetings,
Janneke

-- 
Jan Nieuwenhuizen <janneke@gnu.org> | GNU LilyPond http://lilypond.org
Freelance IT http://JoyofSource.com | Avatar® http://AvatarAcademy.com