Re: [PATCH v9 4/5] xen/arm: switch ARM to use generic implementation of bug.h

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 5 Apr 2023, Julien Grall wrote:
> On 03/04/2023 00:15, Stefano Stabellini wrote:
> > On Fri, 31 Mar 2023, Julien Grall wrote:
> > > Hi Oleksii,
> > > 
> > > I was going to ack the patch but then I spotted something that would want
> > > some
> > > clarification.
> > > 
> > > On 29/03/2023 11:50, Oleksii Kurochko wrote:
> > > > diff --git a/xen/arch/arm/include/asm/bug.h
> > > > b/xen/arch/arm/include/asm/bug.h
> > > > index cacaf014ab..3fb0471a9b 100644
> > > > --- a/xen/arch/arm/include/asm/bug.h
> > > > +++ b/xen/arch/arm/include/asm/bug.h
> > > > @@ -1,6 +1,24 @@
> > > >    #ifndef __ARM_BUG_H__
> > > >    #define __ARM_BUG_H__
> > > >    +/*
> > > > + * Please do not include in the header any header that might
> > > > + * use BUG/ASSERT/etc maros asthey will be defined later after
> > > > + * the return to <xen/bug.h> from the current header:
> > > > + *
> > > > + * <xen/bug.h>:
> > > > + *  ...
> > > > + *   <asm/bug.h>:
> > > > + *     ...
> > > > + *     <any_header_which_uses_BUG/ASSERT/etc macros.h>
> > > > + *     ...
> > > > + *  ...
> > > > + *  #define BUG() ...
> > > > + *  ...
> > > > + *  #define ASSERT() ...
> > > > + *  ...
> > > > + */
> > > > +
> > > >    #include <xen/types.h>
> > > >      #if defined(CONFIG_ARM_32)
> > > > @@ -11,76 +29,7 @@
> > > >    # error "unknown ARM variant"
> > > >    #endif
> > > >    -#define BUG_FRAME_STRUCT
> > > > -
> > > > -struct bug_frame {
> > > > -    signed int loc_disp;    /* Relative address to the bug address */
> > > > -    signed int file_disp;   /* Relative address to the filename */
> > > > -    signed int msg_disp;    /* Relative address to the predicate (for
> > > > ASSERT) */
> > > > -    uint16_t line;          /* Line number */
> > > > -    uint32_t pad0:16;       /* Padding for 8-bytes align */
> > > > -};
> > > > -
> > > > -#define bug_loc(b) ((const void *)(b) + (b)->loc_disp)
> > > > -#define bug_file(b) ((const void *)(b) + (b)->file_disp);
> > > > -#define bug_line(b) ((b)->line)
> > > > -#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
> > > > -
> > > > -/* Many versions of GCC doesn't support the asm %c parameter which
> > > > would
> > > > - * be preferable to this unpleasantness. We use mergeable string
> > > > - * sections to avoid multiple copies of the string appearing in the
> > > > - * Xen image. BUGFRAME_run_fn needs to be handled separately.
> > > > - */
> > > 
> > > Given this comment ...
> > > 
> > > > -#define BUG_FRAME(type, line, file, has_msg, msg) do {
> > > > \
> > > > -    BUILD_BUG_ON((line) >> 16);
> > > > \
> > > > -    BUILD_BUG_ON((type) >= BUGFRAME_NR);
> > > > \
> > > > -    asm ("1:"BUG_INSTR"\n"
> > > > \
> > > > -         ".pushsection .rodata.str, \"aMS\", %progbits, 1\n"
> > > > \
> > > > -         "2:\t.asciz " __stringify(file) "\n"
> > > > \
> > > > -         "3:\n"
> > > > \
> > > > -         ".if " #has_msg "\n"
> > > > \
> > > > -         "\t.asciz " #msg "\n"
> > > > \
> > > > -         ".endif\n"
> > > > \
> > > > -         ".popsection\n"
> > > > \
> > > > -         ".pushsection .bug_frames." __stringify(type) ", \"a\",
> > > > %progbits\n"\
> > > > -         "4:\n"
> > > > \
> > > > -         ".p2align 2\n"
> > > > \
> > > > -         ".long (1b - 4b)\n"
> > > > \
> > > > -         ".long (2b - 4b)\n"
> > > > \
> > > > -         ".long (3b - 4b)\n"
> > > > \
> > > > -         ".hword " __stringify(line) ", 0\n"
> > > > \
> > > > -         ".popsection");
> > > > \
> > > > -} while (0)
> > > > -
> > > > -/*
> > > > - * GCC will not allow to use "i"  when PIE is enabled (Xen doesn't set
> > > > the
> > > > - * flag but instead rely on the default value from the compiler). So
> > > > the
> > > > - * easiest way to implement run_in_exception_handler() is to pass the
> > > > to
> > > > - * be called function in a fixed register.
> > > > - */
> > > > -#define  run_in_exception_handler(fn) do {
> > > > \
> > > > -    asm ("mov " __stringify(BUG_FN_REG) ", %0\n"
> > > > \
> > > > -         "1:"BUG_INSTR"\n"
> > > > \
> > > > -         ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn) ","
> > > > \
> > > > -         "             \"a\", %%progbits\n"
> > > > \
> > > > -         "2:\n"
> > > > \
> > > > -         ".p2align 2\n"
> > > > \
> > > > -         ".long (1b - 2b)\n"
> > > > \
> > > > -         ".long 0, 0, 0\n"
> > > > \
> > > > -         ".popsection" :: "r" (fn) : __stringify(BUG_FN_REG) );
> > > > \
> > > > -} while (0)
> > > > -
> > > > -#define WARN() BUG_FRAME(BUGFRAME_warn, __LINE__, __FILE__, 0, "")
> > > > -
> > > > -#define BUG() do {                                              \
> > > > -    BUG_FRAME(BUGFRAME_bug,  __LINE__, __FILE__, 0, "");        \
> > > > -    unreachable();                                              \
> > > > -} while (0)
> > > > -
> > > > -#define assert_failed(msg) do {                                 \
> > > > -    BUG_FRAME(BUGFRAME_assert, __LINE__, __FILE__, 1, msg);     \
> > > > -    unreachable();                                              \
> > > > -} while (0)
> > > > +#define BUG_ASM_CONST   "c"
> > > 
> > > ... you should explain in the commit message why this is needed and the
> > > problem described above is not a problem anymore.
> > > 
> > > For instance, I managed to build it without 'c' on arm64 [1]. But it does
> > > break on arm32 [2]. I know that Arm is also where '%c' was an issue.
> > > 
> > > Skimming through linux, the reason seems to be that GCC may add '#' when
> > > it
> > > should not. That said, I haven't look at the impact on the generic
> > > implementation. Neither I looked at which version may be affected (the
> > > original message was from 2011).
> > > 
> > > However, without an explanation, I am afraid this can't go in because I am
> > > worry we may break some users (thankfully that might just be a compilation
> > > issues rather than weird behavior).
> > > 
> > > Bertrand, Stefano, do you know if this is still an issue?
> > 
> > I don't know, but I confirm your observation.
> > 
> > In my system, both ARM64 and ARM32 compile without problems with "c".
> > Without "c', only ARM64 compiles without problems, while ARM32 breaks.
> > 
> > My ARM32 compiler is:
> > arm-linux-gnueabihf-gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
> > 
> > Additing a meaningful explanation to the commit message might be
> > difficult in this case.
> 
> Agree. One would need to look at the compiler code to confirm. We should at
> least acknowledge the change in the commit message and also...
> 
> > 
> > Maybe instead we could run a few tests with different versions of arm64
> > and arm32 gcc to check that everything still works? If everything checks
> > out, given that the issue has been unchanged for 10+ years we could just
> > keep "c" and move forward with it?
> 
> ... confirm that we are still able to compile with GCC 4.9 (arm32) and GCC 5.1
> (arm64).
> 
> Do we have those compiler in the CI? (I couldn't easily confirm from the
> automation directory).

In the CI we have:
- arm64v8/alpine:3.12, gcc 9.3.0
- arm64v8/debian:unstable, gcc 12.2.0
- arm64v8/debian:unstable cross arm32, gcc 12.2.0
- yocto, not sure exactly but it is going to be 9.x or newer