Re: [PATCH v8 4/6] Drop gnulib no-abort.patch

[Glenn Washburn]

On Thu, 03 Mar 2022 13:47:29 -0500
Robbie Harwood <rharwood@redhat.com> wrote:

> Glenn Washburn <development@efficientek.com> writes:
> 
> > Robbie Harwood <rharwood@redhat.com> wrote:
> >
> >> If you have a patch that makes this work, I don't have a problem with
> >> it.  However, I was unable to make that work in practice.
> >
> > Can you provide some specifics on what problem you were running in to?
> > Was it a link issue at build time? platform specific? Did it build
> > fine, but blew up in testing? Did you try using the linker options I
> > suggested above?
> 
> No, nor am I about to.  This is code that I have, that builds, that I'm
> submitting to grub.  If you want a different approach, that's fine -
> you're welcome to write a patch to do that instead.  I have built this
> bike shed and I *really* do not care what color it is, nor do I
> appreciate being asked to test other people's proposals for them: you're
> presumably just as capable of building the code yourself and seeing if
> something works or doesn't.  This is v8 of the series and I'm pretty
> much done caring about it at this point.

Sounds like you're frustrated with the process and needed to let off
some stream. I can speak from experience on that subject (I got up to
v9 on a cryptodisk patch series that was actually fixing bugs). Of
course, its fine that you dont appreciate being asked to "test other
people's proposals" and its fine for anyone to ask you to do that,
which I did not (I asked if you had, there's a difference). You seem
capable of setting boundaries, as evidenced here, so no problem there.

There are many people on this list, including me, that have on occasion
modified a patch series according to someone else's proposal. I don't
think its an unreasonable ask, especially when the patch in question is
not correct. You're a free human being capable of saying no, as is your
perogative. There's no need to take it as a personal affront.

The GRUB project seems to me to have a fairly high code quality bar.
I've been frustrated in the past about what I considered bike shedding
as well and have made my frustrations known on and off this list. And I
think this is less trivial than some of those things.

Also, I think a more accurate metaphor, in this case, rather than
quibbling over color is whether you'll get electrocuted when lightening
strikes. This seems like its introducing a misfeature that someone else
down the line will get bitten by. So, if we can avoid that now when we
know there's an issue, the cost is much lower.

I hear you loud and clear saying you're done pushing on this patch
series. Would you like me to take it over? I've found what I believe to
be an acceptable solution using grub_abort.

> 
> For completeness, here's what happens if one just defines to grub_abort
> without further modification:
> 
> $ uname -m
> x86_64
> $ ./bootstrap
> ...
> $ ./configure --enable-grub-mkfont
> ...
> $ make
> ...
> gcc -DHAVE_CONFIG_H -I. -I..  -Wall -W  -DGRUB_MACHINE_PCBIOS=1 
> -DGRUB_MACHINE=I386_PC -m32 -nostdinc -isystem 
> /usr/lib/gcc/x86_64-linux-gnu/11/include -I../include -I../include 
> -DGRUB_FILE=\"lib/gnulib/regex.c\" -I. -I. -I.. -I.. -I../include 
> -I../include -I../grub-core/lib/libgcrypt-grub/src/   
> -I../grub-core/lib/posix_wrap -I../grub-core/lib/gnulib 
> -I../grub-core/lib/gnulib  -D_FILE_OFFSET_BITS=64 -std=gnu99 -Os -m32 -Wall 
> -W -Wshadow -Wpointer-arith -Wundef -Wchar-subscripts -Wcomment 
> -Wdeprecated-declarations -Wdisabled-optimization -Wdiv-by-zero -Wfloat-equal 
> -Wformat-extra-args -Wformat-security -Wformat-y2k -Wimplicit 
> -Wimplicit-function-declaration -Wimplicit-int -Wmain -Wmissing-braces 
> -Wmissing-format-attribute -Wmultichar -Wparentheses -Wreturn-type 
> -Wsequence-point -Wshadow -Wsign-compare -Wswitch -Wtrigraphs 
> -Wunknown-pragmas -Wunused -Wunused-function -Wunused-label 
> -Wunused-parameter -Wunused-value  -Wunused-variable -Wwrite-strings 
> -Wnested-externs -Wstrict-prototypes -g -Wredundant-decls 
> -Wmissing-prototypes -Wmissing-declarations  -Wextra -Wattributes 
> -Wendif-labels -Winit-self -Wint-to-pointer-cast -Winvalid-pch 
> -Wmissing-field-initializers -Wnonnull -Woverflow -Wvla -Wpointer-to-int-cast 
> -Wstrict-aliasing -Wvariadic-macros -Wvolatile-register-var -Wpointer-sign 
> -Wmissing-include-dirs -Wmissing-prototypes -Wmissing-declarations -Wformat=2 
> -march=i386 -mrtd -mregparm=3 -falign-functions=1 -falign-loops=1 
> -falign-jumps=1 -freg-struct-return -mno-mmx -mno-sse -mno-sse2 -mno-sse3 
> -mno-3dnow -Wa,-mx86-used-note=no -msoft-float -fno-dwarf2-cfi-asm 
> -mno-stack-arg-probe -fno-asynchronous-unwind-tables -fno-unwind-tables 
> -fno-ident -fno-PIE -fno-pie -fno-stack-protector -Wtrampolines -Werror   
> -ffreestanding -fno-builtin -Wno-undef -Wno-sign-compare -Wno-unused 
> -Wno-unused-parameter -Wno-redundant-decls -Wno-unreachable-code 
> -Wno-conversion   -MT lib/gnulib/regexp_module-regex.o -MD -MP -MF 
> lib/gnulib/.deps-core/regexp_module-regex.Tpo -c -o 
> lib/gnulib/regexp_module-regex.o `test -f 'lib/gnulib/regex.c' || echo 
> './'`lib/gnulib/regex.c
> In file included from ../grub-core/lib/gnulib/libc-config.h:36,
>                  from lib/gnulib/regex.c:23:
> lib/gnulib/regcomp.c: In function ‘regerror’:
> ../config.h:145:19: error: implicit declaration of function ‘grub_abort’; did 
> you mean ‘grub_reboot’? [-Werror=implicit-function-declaration]
>   145 | #    define abort grub_abort
>       |                   ^~~~~~~~~~
> lib/gnulib/regcomp.c:509:5: note: in expansion of macro ‘abort’
>   509 |     abort ();
>       |     ^~~~~
> ../config.h:145:19: error: nested extern declaration of ‘grub_abort’ 
> [-Werror=nested-externs]
>   145 | #    define abort grub_abort
>       |                   ^~~~~~~~~~
> lib/gnulib/regcomp.c:509:5: note: in expansion of macro ‘abort’
>   509 |     abort ();
>       |     ^~~~~
> cc1: all warnings being treated as errors
> ...
> $ git diff
> diff --git a/config.h.in b/config.h.in
> index 0fca0597d..8ad4aa0ac 100644
> --- a/config.h.in
> +++ b/config.h.in
> @@ -142,7 +142,7 @@ typedef __UINT_FAST32_TYPE__ uint_fast32_t;
>   * a prototype for abort(), so leave this as a macro that doesn't take
>   * arguments.
>   */
> -#    define abort __builtin_trap
> +#    define abort grub_abort

Thank you for letting me know what you tried and what was not working.
It turns out the issue was more complicated and different than I was let
on to believe based on the comment above. And while my link solution
could be part of the solution, its not necessary.

I'll send my changes to this thread shortly, unless you want me to roll
a v9. Thanks for getting this within a hare's breath of the finish line.

Glenn

>  #  endif /* !_GL_INLINE_HEADER_BEGIN */
>  
>  /* gnulib doesn't build cleanly with older compilers. */
> $ 
> 
> Be well,
> --Robbie