[RFC,6/7] arm: Introduce dummy empty functions for data only C files

Commit Message

Andrii Anisov Nov. 6, 2019, 9:19 a.m. UTC

From: Andrii Anisov <[e-mail]>

ARM Compiler 6 has a proven bug: it compiles data only C files with
SoftVFP attributes. This leads to a failed linkage afterwards with
an error:

Error: L6242E: Cannot link object built_in.o as its attributes are incompatible with the image attributes.
... A64 clashes with SoftVFP.

The known workaround is introducing some code into the affected file,
e.g. an empty (non-static) function is enough.

Signed-off-by: Andrii Anisov <[e-mail]>
---
 xen/arch/arm/platforms/brcm-raspberry-pi.c | 2 ++
 xen/arch/arm/platforms/thunderx.c          | 2 ++
 xen/xsm/flask/gen-policy.py                | 4 ++++
 3 files changed, 8 insertions(+)

Comments

Stefano Stabellini Nov. 11, 2019, 8:57 p.m. UTC | #1

On Wed, 6 Nov 2019, Andrii Anisov wrote:
> From: Andrii Anisov <[e-mail]>
> 
> ARM Compiler 6 has a proven bug: it compiles data only C files with
> SoftVFP attributes. This leads to a failed linkage afterwards with
> an error:
> 
> Error: L6242E: Cannot link object built_in.o as its attributes are incompatible with the image attributes.
> ... A64 clashes with SoftVFP.
> 
> The known workaround is introducing some code into the affected file,
> e.g. an empty (non-static) function is enough.

Oh man, this is truly horrible.

If we really have to do this please:

- use the same dummy function name in all files
- the function should be static
- hiding the function within a #ifdef ARMCC block
- potentially hide the whole horrible hack behind a #define so that it
  would become at the call site:

 +ARMCC_DUMMY_FUNC_HACK()



> Signed-off-by: Andrii Anisov <[e-mail]>
>
> ---
>  xen/arch/arm/platforms/brcm-raspberry-pi.c | 2 ++
>  xen/arch/arm/platforms/thunderx.c          | 2 ++
>  xen/xsm/flask/gen-policy.py                | 4 ++++
>  3 files changed, 8 insertions(+)
> 
> diff --git a/xen/arch/arm/platforms/brcm-raspberry-pi.c b/xen/arch/arm/platforms/brcm-raspberry-pi.c
> index b697fa2..7ab1810 100644
> --- a/xen/arch/arm/platforms/brcm-raspberry-pi.c
> +++ b/xen/arch/arm/platforms/brcm-raspberry-pi.c
> @@ -40,6 +40,8 @@ static const struct dt_device_match rpi4_blacklist_dev[] __initconst =
>      { /* sentinel */ },
>  };
>  
> +void brcm_raspberry_pi_dummy_func(void) {}
> +
>  PLATFORM_START(rpi4, "Raspberry Pi 4")
>      .compatible     = rpi4_dt_compat,
>      .blacklist_dev  = rpi4_blacklist_dev,
> diff --git a/xen/arch/arm/platforms/thunderx.c b/xen/arch/arm/platforms/thunderx.c
> index 9b32a29..8015323 100644
> --- a/xen/arch/arm/platforms/thunderx.c
> +++ b/xen/arch/arm/platforms/thunderx.c
> @@ -33,6 +33,8 @@ static const struct dt_device_match thunderx_blacklist_dev[] __initconst =
>      { /* sentinel */ },
>  };
>  
> +void thunderx_dummy_func(void) {}
> +
>  PLATFORM_START(thunderx, "THUNDERX")
>      .compatible = thunderx_dt_compat,
>      .blacklist_dev = thunderx_blacklist_dev,
> diff --git a/xen/xsm/flask/gen-policy.py b/xen/xsm/flask/gen-policy.py
> index c7501e4..73bf7d2 100644
> --- a/xen/xsm/flask/gen-policy.py
> +++ b/xen/xsm/flask/gen-policy.py
> @@ -21,3 +21,7 @@ sys.stdout.write("""
>  };
>  const unsigned int __initconst xsm_flask_init_policy_size = %d;
>  """ % policy_size)
> +
> +sys.stdout.write("""
> +void policy_dummy_func(void) {}
> +""")
> -- 
> 2.7.4
>

Julien Grall Nov. 13, 2019, 5:51 a.m. UTC | #2

Hi,

On Wed, 6 Nov 2019, 18:20 Andrii Anisov, <[e-mail]> wrote:

> From: Andrii Anisov <[e-mail]>
>
> ARM Compiler 6 has a proven bug: it compiles data only C files with
> SoftVFP attributes. This leads to a failed linkage afterwards with
> an error:
>

And there are no way to force disabling the softfvp attributes?


> Error: L6242E: Cannot link object built_in.o as its attributes are
> incompatible with the image attributes.
> ... A64 clashes with SoftVFP.
>
> The known workaround is introducing some code into the affected file,
> e.g. an empty (non-static) function is enough.
>

Was this reported to Arm? If so, what was there answer?

Signed-off-by: Andrii Anisov <[e-mail]>
> ---
>  xen/arch/arm/platforms/brcm-raspberry-pi.c | 2 ++
>  xen/arch/arm/platforms/thunderx.c          | 2 ++
>  xen/xsm/flask/gen-policy.py                | 4 ++++
>  3 files changed, 8 insertions(+)
>
> diff --git a/xen/arch/arm/platforms/brcm-raspberry-pi.c
> b/xen/arch/arm/platforms/brcm-raspberry-pi.c
> index b697fa2..7ab1810 100644
> --- a/xen/arch/arm/platforms/brcm-raspberry-pi.c
> +++ b/xen/arch/arm/platforms/brcm-raspberry-pi.c
> @@ -40,6 +40,8 @@ static const struct dt_device_match rpi4_blacklist_dev[]
> __initconst =
>      { /* sentinel */ },
>  };
>
> +void brcm_raspberry_pi_dummy_func(void) {}
> +
>  PLATFORM_START(rpi4, "Raspberry Pi 4")
>      .compatible     = rpi4_dt_compat,
>      .blacklist_dev  = rpi4_blacklist_dev,
> diff --git a/xen/arch/arm/platforms/thunderx.c
> b/xen/arch/arm/platforms/thunderx.c
> index 9b32a29..8015323 100644
> --- a/xen/arch/arm/platforms/thunderx.c
> +++ b/xen/arch/arm/platforms/thunderx.c
> @@ -33,6 +33,8 @@ static const struct dt_device_match
> thunderx_blacklist_dev[] __initconst =
>      { /* sentinel */ },
>  };
>
> +void thunderx_dummy_func(void) {}
> +
>  PLATFORM_START(thunderx, "THUNDERX")
>      .compatible = thunderx_dt_compat,
>      .blacklist_dev = thunderx_blacklist_dev,
> diff --git a/xen/xsm/flask/gen-policy.py b/xen/xsm/flask/gen-policy.py
> index c7501e4..73bf7d2 100644
> --- a/xen/xsm/flask/gen-policy.py
> +++ b/xen/xsm/flask/gen-policy.py
> @@ -21,3 +21,7 @@ sys.stdout.write("""
>  };
>  const unsigned int __initconst xsm_flask_init_policy_size = %d;
>  """ % policy_size)
> +
> +sys.stdout.write("""
> +void policy_dummy_func(void) {}
> +""")
> --
> 2.7.4
>
>

Andrii Anisov Nov. 13, 2019, 4:41 p.m. UTC | #3

Hello Stefano,

On 11.11.19 22:57, Stefano Stabellini wrote:
> Oh man, this is truly horrible.

I feel your pain.

> 
> If we really have to do this please:
> 
> - use the same dummy function name in all files

No, because

> - the function should be static

those functions will not handle the case if are static.

ARM commented the issue in the correspondent support case:

"A known workaround is to edit the source file and add an unused empty function, as you’ve already found. By default, an unused empty function should be removed from the final image by the unused section elimination feature of the linker, so it shouldn’t have a code size impact.

A command-line option workaround isn’t available. We’ll now set this case to a “Defect/Enhancement” state while the issue remains unfixed, and will keep you updated accordingly."


> - hiding the function within a #ifdef ARMCC block
> - potentially hide the whole horrible hack behind a #define so that it
>    would become at the call site:
> 
>   +ARMCC_DUMMY_FUNC_HACK()

This is quite reasonable.

Andrii Anisov Nov. 13, 2019, 5:07 p.m. UTC | #4

Dear Julien,

On 13.11.19 07:51, Julien Grall wrote:
> Was this reported to Arm? 
All mentioned ARM Compiler issues were reported to ARM. Unfortunately, ARM hesitated to discus them in community, yet asked to open support cases, e.g. [1].
All issues and their WA's were acknowledged by ARM in correspondent support cases. Except C-style shifts issue what is not a bug but the documented feature [2], and ARM's answer about it was really uncertain.
Only after getting answer for all cases I finalized the patches.


> If so, what was there answer?

I already cited the answer for this particular issue while answering Stefano.

[1] https://community.arm.com/developer/tools-software/tools/f/arm-compilers-forum/44287/arm-compiler-6-compiles-data-only-c-file-with-softvfp-attribute
[2] https://developer.arm.com/docs/100070/0612/scatter-file-syntax/expression-evaluation-in-scatter-files/expression-rules-in-scatter-files

Julien Grall Nov. 13, 2019, 11:03 p.m. UTC | #5

On Tue, 12 Nov 2019, 05:57 Stefano Stabellini, <[e-mail]>
wrote:

> On Wed, 6 Nov 2019, Andrii Anisov wrote:
> > From: Andrii Anisov <[e-mail]>
> >
> > ARM Compiler 6 has a proven bug: it compiles data only C files with
> > SoftVFP attributes. This leads to a failed linkage afterwards with
> > an error:
> >
> > Error: L6242E: Cannot link object built_in.o as its attributes are
> incompatible with the image attributes.
> > ... A64 clashes with SoftVFP.
> >
> > The known workaround is introducing some code into the affected file,
> > e.g. an empty (non-static) function is enough.
>
> Oh man, this is truly horrible.
>
> If we really have to do this please:
>
> - use the same dummy function name in all files
> - the function should be static
> - hiding the function within a #ifdef ARMCC block
> - potentially hide the whole horrible hack behind a #define so that it
>   would become at the call site:
>
>  +ARMCC_DUMMY_FUNC_HACK()


The risk here is we may introduce new file in the future possibly in common
code with similar issues. So I would prefer if we can find an automatic way
to do this. Some ideas:
    - Add the function at compile time (via makefile). This would be done
for all the files but that's should not be a major issues.
    - Force disable softfvp either via command line, new line in the code
or rewriting the attribute of the object.

But before spending time trying to workaround a buggy compiler. What's the
plan with it? Is it going to be used in production or just a demo?

Cheers,

Artem Mygaiev Nov. 14, 2019, 1:32 p.m. UTC | #6

Hello Julien

On Thu, 2019-11-14 at 08:03 +0900, Julien Grall wrote:
> 
> 
> On Tue, 12 Nov 2019, 05:57 Stefano Stabellini, <
> [e-mail]> wrote:
> > On Wed, 6 Nov 2019, Andrii Anisov wrote:
> > > From: Andrii Anisov <[e-mail]>
> > > 
> > > ARM Compiler 6 has a proven bug: it compiles data only C files
> > with
> > > SoftVFP attributes. This leads to a failed linkage afterwards
> > with
> > > an error:
> > > 
> > > Error: L6242E: Cannot link object built_in.o as its attributes
> > are incompatible with the image attributes.
> > > ... A64 clashes with SoftVFP.
> > > 
> > > The known workaround is introducing some code into the affected
> > file,
> > > e.g. an empty (non-static) function is enough.
> > 
> > Oh man, this is truly horrible.
> > 
> > If we really have to do this please:
> > 
> > - use the same dummy function name in all files
> > - the function should be static
> > - hiding the function within a #ifdef ARMCC block
> > - potentially hide the whole horrible hack behind a #define so that
> > it
> >   would become at the call site:
> > 
> >  +ARMCC_DUMMY_FUNC_HACK()
> 
> 
> The risk here is we may introduce new file in the future possibly in
> common code with similar issues. So I would prefer if we can find an
> automatic way to do this. Some ideas:
>     - Add the function at compile time (via makefile). This would be
> done for all the files but that's should not be a major issues.
>     - Force disable softfvp either via command line, new line in the
> code or rewriting the attribute of the object.
> 
> But before spending time trying to workaround a buggy compiler.
> What's the plan with it? Is it going to be used in production or just
> a demo?

This is not intended for a production program at the moment, and it
obviously require lot of further work. I would not try to upstream ugly
workarounds for issues like the one above, it would be much better to
somehow persuade Arm tools team to properly fix them.

This RFC series has following goals:
1) prove that we can use safety-certified tools for Xen and avoid
possible arguments on compiler/linker certification path
2) research possible issues when using non-standard compiler/linker and
try to see if it is easy to adjust Xen to use them

In the end, it would be great to make Xen build system flexible enough
to use with non-standard compilers without overcomplicating it or changing it significantly, causing too much disruption to community.

> Cheers,
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> [e-mail]
> 
> https://urldefense.com/v3/__https://lists.xenproject.org/mailman/listinfo/xen-devel__;!K6dmGCEab4ueJg!kCpXu2prtUxCHZV8aCvxYk9E82KnsHuNftyCeG745Ei3vhO2VP_SYXDnItHeZZCwdw$
>

Stefano Stabellini Nov. 20, 2019, 10:20 p.m. UTC | #7

+ fusa-sig

On Thu, 14 Nov 2019, Artem Mygaiev wrote:
> Hello Julien
> 
> On Thu, 2019-11-14 at 08:03 +0900, Julien Grall wrote:
> > 
> > 
> > On Tue, 12 Nov 2019, 05:57 Stefano Stabellini, <
> > [e-mail]> wrote:
> > > On Wed, 6 Nov 2019, Andrii Anisov wrote:
> > > > From: Andrii Anisov <[e-mail]>
> > > > 
> > > > ARM Compiler 6 has a proven bug: it compiles data only C files
> > > with
> > > > SoftVFP attributes. This leads to a failed linkage afterwards
> > > with
> > > > an error:
> > > > 
> > > > Error: L6242E: Cannot link object built_in.o as its attributes
> > > are incompatible with the image attributes.
> > > > ... A64 clashes with SoftVFP.
> > > > 
> > > > The known workaround is introducing some code into the affected
> > > file,
> > > > e.g. an empty (non-static) function is enough.
> > > 
> > > Oh man, this is truly horrible.
> > > 
> > > If we really have to do this please:
> > > 
> > > - use the same dummy function name in all files
> > > - the function should be static
> > > - hiding the function within a #ifdef ARMCC block
> > > - potentially hide the whole horrible hack behind a #define so that
> > > it
> > >   would become at the call site:
> > > 
> > >  +ARMCC_DUMMY_FUNC_HACK()
> > 
> > 
> > The risk here is we may introduce new file in the future possibly in
> > common code with similar issues. So I would prefer if we can find an
> > automatic way to do this. Some ideas:
> >     - Add the function at compile time (via makefile). This would be
> > done for all the files but that's should not be a major issues.
> >     - Force disable softfvp either via command line, new line in the
> > code or rewriting the attribute of the object.
> > 
> > But before spending time trying to workaround a buggy compiler.
> > What's the plan with it? Is it going to be used in production or just
> > a demo?
> 
> This is not intended for a production program at the moment, and it
> obviously require lot of further work. I would not try to upstream ugly
> workarounds for issues like the one above, it would be much better to
> somehow persuade Arm tools team to properly fix them.
> 
> This RFC series has following goals:
> 1) prove that we can use safety-certified tools for Xen and avoid
> possible arguments on compiler/linker certification path
> 2) research possible issues when using non-standard compiler/linker and
> try to see if it is easy to adjust Xen to use them
> 
> In the end, it would be great to make Xen build system flexible enough
> to use with non-standard compilers without overcomplicating it or changing it significantly, causing too much disruption to community.

I am aligned with you on the goals.

On this specific issue, it would be great if Arm fixed their compiler.
Maybe we could discussed this problem with the Arm folks during one of
the next FuSa calls (list in CC).

Patch

diff --git a/xen/arch/arm/platforms/brcm-raspberry-pi.c b/xen/arch/arm/platforms/brcm-raspberry-pi.c
index b697fa2..7ab1810 100644
--- a/xen/arch/arm/platforms/brcm-raspberry-pi.c
+++ b/xen/arch/arm/platforms/brcm-raspberry-pi.c
@@ -40,6 +40,8 @@  static const struct dt_device_match rpi4_blacklist_dev[] __initconst =
     { /* sentinel */ },
 };
 
+void brcm_raspberry_pi_dummy_func(void) {}
+
 PLATFORM_START(rpi4, "Raspberry Pi 4")
     .compatible     = rpi4_dt_compat,
     .blacklist_dev  = rpi4_blacklist_dev,
diff --git a/xen/arch/arm/platforms/thunderx.c b/xen/arch/arm/platforms/thunderx.c
index 9b32a29..8015323 100644
--- a/xen/arch/arm/platforms/thunderx.c
+++ b/xen/arch/arm/platforms/thunderx.c
@@ -33,6 +33,8 @@  static const struct dt_device_match thunderx_blacklist_dev[] __initconst =
     { /* sentinel */ },
 };
 
+void thunderx_dummy_func(void) {}
+
 PLATFORM_START(thunderx, "THUNDERX")
     .compatible = thunderx_dt_compat,
     .blacklist_dev = thunderx_blacklist_dev,
diff --git a/xen/xsm/flask/gen-policy.py b/xen/xsm/flask/gen-policy.py
index c7501e4..73bf7d2 100644
--- a/xen/xsm/flask/gen-policy.py
+++ b/xen/xsm/flask/gen-policy.py
@@ -21,3 +21,7 @@  sys.stdout.write("""
 };
 const unsigned int __initconst xsm_flask_init_policy_size = %d;
 """ % policy_size)
+
+sys.stdout.write("""
+void policy_dummy_func(void) {}
+""")