[PATCH 0/3] fixes for CLANG35 on ARM

[PATCH 0/3] fixes for CLANG35 on ARM

[Ard Biesheuvel]

Building with Clang 3.5 for ARM may result in build breakage, due to the
fact that it may emit non-adjacent movw/movt instructions pairs which
cannot be relocated in PE/COFF. We pass -mno-movt in some places to
work around a related issue in the relocatable PrePi in ArmVirtPkg, but
we need to disable movw/movt entirely to really address this issue.

So first, fix some breakage that results from building with -mlong-calls
in the optimized BaseMemoryLib code (#1)

Patch #2 switches to -mkernel, which disables movw/movt generation (and
enabled -mlong-calls as a side effect)

Patch #3 removes the now redundant, and incompatible command line
overrides for the relocatable PrePi.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Laszlo Ersek <lersek@redhat.com>

Ard Biesheuvel (3):
  MdePkg/BaseMemoryLibOptDxe ARM: add missing function annotations
  BaseTools/tools_def ARM CLANG35: work around -mno-movt option name
    change
  ArmVirtPkg/PrePi ARM CLANG35: drop incompatible command line option

 ArmVirtPkg/ArmVirtQemuKernel.dsc                     | 5 -----
 ArmVirtPkg/ArmVirtXen.dsc                            | 5 -----
 BaseTools/Conf/tools_def.template                    | 2 +-
 MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S | 1 +
 MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S  | 1 +
 MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S     | 1 +
 MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S      | 5 +++++
 7 files changed, 9 insertions(+), 11 deletions(-)

-- 
2.19.2

[PATCH 1/3] MdePkg/BaseMemoryLibOptDxe ARM: add missing function annotations

[Ard Biesheuvel]

ARM uses the low order bit of a branch target address to decide in
which execution mode (ARM or Thumb) a function needs to be called.
In order for this to work across object files, ELF function symbols
will have the low bit set if they were emitted in Thumb mode and
cleared otherwise. This annotation is only emitted if the ELF symbols
are annotated as function, since taking the address of some data
symbol (e.g., a literal) should not produce a value with the low bit
set, even if it appears in an object file containing Thumb code.

This means that all functions coded in assembler must have this
function annotation, or they may end up getting called in the
wrong mode, crashing the program.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S | 1 +
 MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S  | 1 +
 MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S     | 1 +
 MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S      | 5 +++++
 4 files changed, 8 insertions(+)

diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S
index 6d0089049d48..b74056fa1f5f 100644
--- a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S
@@ -30,6 +30,7 @@
     .thumb
     .syntax unified
     .align  5
+    .type   ASM_PFX(InternalMemCompareGuid), %function
 ASM_GLOBAL ASM_PFX(InternalMemCompareGuid)
 ASM_PFX(InternalMemCompareGuid):
     push    {r4, lr}
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S
index 9483aab61a0c..25a9a0994524 100644
--- a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S
@@ -46,6 +46,7 @@
     .thumb
     .syntax unified
     .align  5
+    .type   ASM_PFX(InternalMemCompareMem), %function
 ASM_GLOBAL ASM_PFX(InternalMemCompareMem)
 ASM_PFX(InternalMemCompareMem):
     push    {r4-r8, lr}
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S
index 195a0b23f770..e1543f3c2a43 100644
--- a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S
@@ -42,6 +42,7 @@ InternalMemCopyMem (
   IN      UINTN                     Length
   )
 **/
+    .type   ASM_PFX(InternalMemCopyMem), %function
 ASM_GLOBAL ASM_PFX(InternalMemCopyMem)
 ASM_PFX(InternalMemCopyMem):
     push    {r4-r11, lr}
diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S
index 2d8f4d5b8621..928c1a12d558 100644
--- a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S
+++ b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S
@@ -16,6 +16,7 @@
     .thumb
     .syntax unified
     .align  5
+    .type   ASM_PFX(InternalMemSetMem16), %function
 ASM_GLOBAL ASM_PFX(InternalMemSetMem16)
 ASM_PFX(InternalMemSetMem16):
     uxth    r2, r2
@@ -23,17 +24,20 @@ ASM_PFX(InternalMemSetMem16):
     orr     r2, r2, r2, lsl #16
     b       0f
 
+    .type   ASM_PFX(InternalMemSetMem32), %function
 ASM_GLOBAL ASM_PFX(InternalMemSetMem32)
 ASM_PFX(InternalMemSetMem32):
     lsl     r1, r1, #2
     b       0f
 
+    .type   ASM_PFX(InternalMemSetMem64), %function
 ASM_GLOBAL ASM_PFX(InternalMemSetMem64)
 ASM_PFX(InternalMemSetMem64):
     lsl     r1, r1, #3
     b       1f
 
     .align  5
+    .type   ASM_PFX(InternalMemSetMem), %function
 ASM_GLOBAL ASM_PFX(InternalMemSetMem)
 ASM_PFX(InternalMemSetMem):
     uxtb    r2, r2
@@ -41,6 +45,7 @@ ASM_PFX(InternalMemSetMem):
     orr     r2, r2, r2, lsl #16
     b       0f
 
+    .type   ASM_PFX(InternalMemZeroMem), %function
 ASM_GLOBAL ASM_PFX(InternalMemZeroMem)
 ASM_PFX(InternalMemZeroMem):
     movs    r2, #0
-- 
2.19.2

Re: [PATCH 1/3] MdePkg/BaseMemoryLibOptDxe ARM: add missing function annotations

[Laszlo Ersek]

On 12/12/18 11:33, Ard Biesheuvel wrote:
> ARM uses the low order bit of a branch target address to decide in
> which execution mode (ARM or Thumb) a function needs to be called.
> In order for this to work across object files, ELF function symbols
> will have the low bit set if they were emitted in Thumb mode and
> cleared otherwise. This annotation is only emitted if the ELF symbols
> are annotated as function, since taking the address of some data
> symbol (e.g., a literal) should not produce a value with the low bit
> set, even if it appears in an object file containing Thumb code.
> 
> This means that all functions coded in assembler must have this
> function annotation, or they may end up getting called in the
> wrong mode, crashing the program.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S | 1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S  | 1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S     | 1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S      | 5 +++++
>  4 files changed, 8 insertions(+)
> 
> diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S
> index 6d0089049d48..b74056fa1f5f 100644
> --- a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S
> +++ b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S
> @@ -30,6 +30,7 @@
>      .thumb
>      .syntax unified
>      .align  5
> +    .type   ASM_PFX(InternalMemCompareGuid), %function
>  ASM_GLOBAL ASM_PFX(InternalMemCompareGuid)
>  ASM_PFX(InternalMemCompareGuid):
>      push    {r4, lr}
> diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S
> index 9483aab61a0c..25a9a0994524 100644
> --- a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S
> +++ b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S
> @@ -46,6 +46,7 @@
>      .thumb
>      .syntax unified
>      .align  5
> +    .type   ASM_PFX(InternalMemCompareMem), %function
>  ASM_GLOBAL ASM_PFX(InternalMemCompareMem)
>  ASM_PFX(InternalMemCompareMem):
>      push    {r4-r8, lr}
> diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S
> index 195a0b23f770..e1543f3c2a43 100644
> --- a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S
> +++ b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S
> @@ -42,6 +42,7 @@ InternalMemCopyMem (
>    IN      UINTN                     Length
>    )
>  **/
> +    .type   ASM_PFX(InternalMemCopyMem), %function
>  ASM_GLOBAL ASM_PFX(InternalMemCopyMem)
>  ASM_PFX(InternalMemCopyMem):
>      push    {r4-r11, lr}
> diff --git a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S
> index 2d8f4d5b8621..928c1a12d558 100644
> --- a/MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S
> +++ b/MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S
> @@ -16,6 +16,7 @@
>      .thumb
>      .syntax unified
>      .align  5
> +    .type   ASM_PFX(InternalMemSetMem16), %function
>  ASM_GLOBAL ASM_PFX(InternalMemSetMem16)
>  ASM_PFX(InternalMemSetMem16):
>      uxth    r2, r2
> @@ -23,17 +24,20 @@ ASM_PFX(InternalMemSetMem16):
>      orr     r2, r2, r2, lsl #16
>      b       0f
>  
> +    .type   ASM_PFX(InternalMemSetMem32), %function
>  ASM_GLOBAL ASM_PFX(InternalMemSetMem32)
>  ASM_PFX(InternalMemSetMem32):
>      lsl     r1, r1, #2
>      b       0f
>  
> +    .type   ASM_PFX(InternalMemSetMem64), %function
>  ASM_GLOBAL ASM_PFX(InternalMemSetMem64)
>  ASM_PFX(InternalMemSetMem64):
>      lsl     r1, r1, #3
>      b       1f
>  
>      .align  5
> +    .type   ASM_PFX(InternalMemSetMem), %function
>  ASM_GLOBAL ASM_PFX(InternalMemSetMem)
>  ASM_PFX(InternalMemSetMem):
>      uxtb    r2, r2
> @@ -41,6 +45,7 @@ ASM_PFX(InternalMemSetMem):
>      orr     r2, r2, r2, lsl #16
>      b       0f
>  
> +    .type   ASM_PFX(InternalMemZeroMem), %function
>  ASM_GLOBAL ASM_PFX(InternalMemZeroMem)
>  ASM_PFX(InternalMemZeroMem):
>      movs    r2, #0
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>

[PATCH 2/3] BaseTools/tools_def ARM CLANG35: work around -mno-movt option name change

[Ard Biesheuvel]

PE/COFF only has a very limited id space for runtime relocations, and
so it defines only a single relocation for movw/movt instruction pairs,
which can be combined to load a 32-bit symbol reference into a register.
For this to work as expected, these instructions must always appear in
the same order and adjacently, and this is something few compilers take
into account, unless they target PE/COFF explicitly (and this is not the
case for our ELF based toolchains)

For Clang 3.6 and later, we can pass the -mno-movt option to suppress
movw/movt pairs entirely, which works around the issue. Unfortunately,
for Clang 3.5, the option is called differently (-mllvm -arm-use-movt=0)
and mutually incompatible between 3.5 and 3.6.

Since it is desirable for the CLANG35 toolchain to be usable on newer
versions of Clang as well (given that it is the only non-LTO alternative
to CLANG38), let's work around this issue in a way that permits versions
3.5 and newer of Clang to be used with the CLANG35 profile.

So pass the -mkernel flag instead (and drop the -mno-unaligned-access
in *_CLANG35_ARM_CC_XIPFLAGS which now becomes redundant, and which
Clang complains about). This also inhibits movw/movt generation, along
with some other changes (e.g., long calls) which do affect code generation
but not in a undesirable manner.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 BaseTools/Conf/tools_def.template | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index ac2b95e0f5ba..2ba833e1fb06 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -5249,7 +5249,7 @@ DEFINE CLANG35_AARCH64_CC_FLAGS  = DEF(GCC_AARCH64_CC_FLAGS) DEF(CLANG35_AARCH64
 *_CLANG35_ARM_ASM_FLAGS          = DEF(GCC_ASM_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) -Qunused-arguments
 *_CLANG35_ARM_DLINK_FLAGS        = DEF(CLANG35_ARM_TARGET) DEF(GCC_ARM_DLINK_FLAGS)
 *_CLANG35_ARM_DLINK2_FLAGS       = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220
-*_CLANG35_ARM_PLATFORM_FLAGS     = -march=armv7-a
+*_CLANG35_ARM_PLATFORM_FLAGS     = -march=armv7-a -mkernel -Qunused-arguments
 *_CLANG35_ARM_PP_FLAGS           = DEF(GCC_PP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
 *_CLANG35_ARM_RC_FLAGS           = DEF(GCC_ARM_RC_FLAGS)
 *_CLANG35_ARM_VFRPP_FLAGS        = DEF(GCC_VFRPP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
-- 
2.19.2

Re: [PATCH 2/3] BaseTools/tools_def ARM CLANG35: work around -mno-movt option name change

[Laszlo Ersek]

On 12/12/18 11:33, Ard Biesheuvel wrote:
> PE/COFF only has a very limited id space for runtime relocations, and
> so it defines only a single relocation for movw/movt instruction pairs,
> which can be combined to load a 32-bit symbol reference into a register.
> For this to work as expected, these instructions must always appear in
> the same order and adjacently, and this is something few compilers take
> into account, unless they target PE/COFF explicitly (and this is not the
> case for our ELF based toolchains)
> 
> For Clang 3.6 and later, we can pass the -mno-movt option to suppress
> movw/movt pairs entirely, which works around the issue. Unfortunately,
> for Clang 3.5, the option is called differently (-mllvm -arm-use-movt=0)
> and mutually incompatible between 3.5 and 3.6.
> 
> Since it is desirable for the CLANG35 toolchain to be usable on newer
> versions of Clang as well (given that it is the only non-LTO alternative
> to CLANG38), let's work around this issue in a way that permits versions
> 3.5 and newer of Clang to be used with the CLANG35 profile.
> 
> So pass the -mkernel flag instead (and drop the -mno-unaligned-access
> in *_CLANG35_ARM_CC_XIPFLAGS which now becomes redundant, and which
> Clang complains about). This also inhibits movw/movt generation, along
> with some other changes (e.g., long calls) which do affect code generation
> but not in a undesirable manner.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  BaseTools/Conf/tools_def.template | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> index ac2b95e0f5ba..2ba833e1fb06 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -5249,7 +5249,7 @@ DEFINE CLANG35_AARCH64_CC_FLAGS  = DEF(GCC_AARCH64_CC_FLAGS) DEF(CLANG35_AARCH64
>  *_CLANG35_ARM_ASM_FLAGS          = DEF(GCC_ASM_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) -Qunused-arguments
>  *_CLANG35_ARM_DLINK_FLAGS        = DEF(CLANG35_ARM_TARGET) DEF(GCC_ARM_DLINK_FLAGS)
>  *_CLANG35_ARM_DLINK2_FLAGS       = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220
> -*_CLANG35_ARM_PLATFORM_FLAGS     = -march=armv7-a
> +*_CLANG35_ARM_PLATFORM_FLAGS     = -march=armv7-a -mkernel -Qunused-arguments
>  *_CLANG35_ARM_PP_FLAGS           = DEF(GCC_PP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
>  *_CLANG35_ARM_RC_FLAGS           = DEF(GCC_ARM_RC_FLAGS)
>  *_CLANG35_ARM_VFRPP_FLAGS        = DEF(GCC_VFRPP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
> 

The commit message speaks about adding -mkernel, and removing
-mno-unaligned-access (elsewhere). In the patch, I only see -mkernel
(plus an un-announced -Qunused-arguments option).

Is the commit message slightly out-of-date relative to the code? (Or
perhaps the other way around?)

Thanks!
Laszlo

Re: [PATCH 2/3] BaseTools/tools_def ARM CLANG35: work around -mno-movt option name change

[Ard Biesheuvel]

On Wed, 12 Dec 2018 at 12:50, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 12/12/18 11:33, Ard Biesheuvel wrote:
> > PE/COFF only has a very limited id space for runtime relocations, and
> > so it defines only a single relocation for movw/movt instruction pairs,
> > which can be combined to load a 32-bit symbol reference into a register.
> > For this to work as expected, these instructions must always appear in
> > the same order and adjacently, and this is something few compilers take
> > into account, unless they target PE/COFF explicitly (and this is not the
> > case for our ELF based toolchains)
> >
> > For Clang 3.6 and later, we can pass the -mno-movt option to suppress
> > movw/movt pairs entirely, which works around the issue. Unfortunately,
> > for Clang 3.5, the option is called differently (-mllvm -arm-use-movt=0)
> > and mutually incompatible between 3.5 and 3.6.
> >
> > Since it is desirable for the CLANG35 toolchain to be usable on newer
> > versions of Clang as well (given that it is the only non-LTO alternative
> > to CLANG38), let's work around this issue in a way that permits versions
> > 3.5 and newer of Clang to be used with the CLANG35 profile.
> >
> > So pass the -mkernel flag instead (and drop the -mno-unaligned-access
> > in *_CLANG35_ARM_CC_XIPFLAGS which now becomes redundant, and which
> > Clang complains about). This also inhibits movw/movt generation, along
> > with some other changes (e.g., long calls) which do affect code generation
> > but not in a undesirable manner.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  BaseTools/Conf/tools_def.template | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> > index ac2b95e0f5ba..2ba833e1fb06 100755
> > --- a/BaseTools/Conf/tools_def.template
> > +++ b/BaseTools/Conf/tools_def.template
> > @@ -5249,7 +5249,7 @@ DEFINE CLANG35_AARCH64_CC_FLAGS  = DEF(GCC_AARCH64_CC_FLAGS) DEF(CLANG35_AARCH64
> >  *_CLANG35_ARM_ASM_FLAGS          = DEF(GCC_ASM_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) -Qunused-arguments
> >  *_CLANG35_ARM_DLINK_FLAGS        = DEF(CLANG35_ARM_TARGET) DEF(GCC_ARM_DLINK_FLAGS)
> >  *_CLANG35_ARM_DLINK2_FLAGS       = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220
> > -*_CLANG35_ARM_PLATFORM_FLAGS     = -march=armv7-a
> > +*_CLANG35_ARM_PLATFORM_FLAGS     = -march=armv7-a -mkernel -Qunused-arguments
> >  *_CLANG35_ARM_PP_FLAGS           = DEF(GCC_PP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
> >  *_CLANG35_ARM_RC_FLAGS           = DEF(GCC_ARM_RC_FLAGS)
> >  *_CLANG35_ARM_VFRPP_FLAGS        = DEF(GCC_VFRPP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
> >
>
> The commit message speaks about adding -mkernel, and removing
> -mno-unaligned-access (elsewhere). In the patch, I only see -mkernel
> (plus an un-announced -Qunused-arguments option).
>
> Is the commit message slightly out-of-date relative to the code? (Or
> perhaps the other way around?)
>

Ah yes. The  -mno-unaligned-access became redundant, but instead of
removing it, i ended up adding the -Qunused-arguments to stop Clang
from complaining about it.

Re: [PATCH 2/3] BaseTools/tools_def ARM CLANG35: work around -mno-movt option name change

[Laszlo Ersek]

On 12/12/18 12:51, Ard Biesheuvel wrote:
> On Wed, 12 Dec 2018 at 12:50, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 12/12/18 11:33, Ard Biesheuvel wrote:
>>> PE/COFF only has a very limited id space for runtime relocations, and
>>> so it defines only a single relocation for movw/movt instruction pairs,
>>> which can be combined to load a 32-bit symbol reference into a register.
>>> For this to work as expected, these instructions must always appear in
>>> the same order and adjacently, and this is something few compilers take
>>> into account, unless they target PE/COFF explicitly (and this is not the
>>> case for our ELF based toolchains)
>>>
>>> For Clang 3.6 and later, we can pass the -mno-movt option to suppress
>>> movw/movt pairs entirely, which works around the issue. Unfortunately,
>>> for Clang 3.5, the option is called differently (-mllvm -arm-use-movt=0)
>>> and mutually incompatible between 3.5 and 3.6.
>>>
>>> Since it is desirable for the CLANG35 toolchain to be usable on newer
>>> versions of Clang as well (given that it is the only non-LTO alternative
>>> to CLANG38), let's work around this issue in a way that permits versions
>>> 3.5 and newer of Clang to be used with the CLANG35 profile.
>>>
>>> So pass the -mkernel flag instead (and drop the -mno-unaligned-access
>>> in *_CLANG35_ARM_CC_XIPFLAGS which now becomes redundant, and which
>>> Clang complains about). This also inhibits movw/movt generation, along
>>> with some other changes (e.g., long calls) which do affect code generation
>>> but not in a undesirable manner.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  BaseTools/Conf/tools_def.template | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
>>> index ac2b95e0f5ba..2ba833e1fb06 100755
>>> --- a/BaseTools/Conf/tools_def.template
>>> +++ b/BaseTools/Conf/tools_def.template
>>> @@ -5249,7 +5249,7 @@ DEFINE CLANG35_AARCH64_CC_FLAGS  = DEF(GCC_AARCH64_CC_FLAGS) DEF(CLANG35_AARCH64
>>>  *_CLANG35_ARM_ASM_FLAGS          = DEF(GCC_ASM_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) -Qunused-arguments
>>>  *_CLANG35_ARM_DLINK_FLAGS        = DEF(CLANG35_ARM_TARGET) DEF(GCC_ARM_DLINK_FLAGS)
>>>  *_CLANG35_ARM_DLINK2_FLAGS       = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220
>>> -*_CLANG35_ARM_PLATFORM_FLAGS     = -march=armv7-a
>>> +*_CLANG35_ARM_PLATFORM_FLAGS     = -march=armv7-a -mkernel -Qunused-arguments
>>>  *_CLANG35_ARM_PP_FLAGS           = DEF(GCC_PP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
>>>  *_CLANG35_ARM_RC_FLAGS           = DEF(GCC_ARM_RC_FLAGS)
>>>  *_CLANG35_ARM_VFRPP_FLAGS        = DEF(GCC_VFRPP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
>>>
>>
>> The commit message speaks about adding -mkernel, and removing
>> -mno-unaligned-access (elsewhere). In the patch, I only see -mkernel
>> (plus an un-announced -Qunused-arguments option).
>>
>> Is the commit message slightly out-of-date relative to the code? (Or
>> perhaps the other way around?)
>>
> 
> Ah yes. The  -mno-unaligned-access became redundant, but instead of
> removing it, i ended up adding the -Qunused-arguments to stop Clang
> from complaining about it.
> 

OK, thanks! With the commit message updated accordingly:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 2/3] BaseTools/tools_def ARM CLANG35: work around -mno-movt option name change

[Ard Biesheuvel]

On Wed, 12 Dec 2018 at 11:33, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> PE/COFF only has a very limited id space for runtime relocations, and
> so it defines only a single relocation for movw/movt instruction pairs,
> which can be combined to load a 32-bit symbol reference into a register.
> For this to work as expected, these instructions must always appear in
> the same order and adjacently, and this is something few compilers take
> into account, unless they target PE/COFF explicitly (and this is not the
> case for our ELF based toolchains)
>
> For Clang 3.6 and later, we can pass the -mno-movt option to suppress
> movw/movt pairs entirely, which works around the issue. Unfortunately,
> for Clang 3.5, the option is called differently (-mllvm -arm-use-movt=0)
> and mutually incompatible between 3.5 and 3.6.
>
> Since it is desirable for the CLANG35 toolchain to be usable on newer
> versions of Clang as well (given that it is the only non-LTO alternative
> to CLANG38), let's work around this issue in a way that permits versions
> 3.5 and newer of Clang to be used with the CLANG35 profile.
>
> So pass the -mkernel flag instead (and drop the -mno-unaligned-access
> in *_CLANG35_ARM_CC_XIPFLAGS which now becomes redundant, and which
> Clang complains about). This also inhibits movw/movt generation, along
> with some other changes (e.g., long calls) which do affect code generation
> but not in a undesirable manner.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  BaseTools/Conf/tools_def.template | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> index ac2b95e0f5ba..2ba833e1fb06 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -5249,7 +5249,7 @@ DEFINE CLANG35_AARCH64_CC_FLAGS  = DEF(GCC_AARCH64_CC_FLAGS) DEF(CLANG35_AARCH64
>  *_CLANG35_ARM_ASM_FLAGS          = DEF(GCC_ASM_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) -Qunused-arguments
>  *_CLANG35_ARM_DLINK_FLAGS        = DEF(CLANG35_ARM_TARGET) DEF(GCC_ARM_DLINK_FLAGS)
>  *_CLANG35_ARM_DLINK2_FLAGS       = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220
> -*_CLANG35_ARM_PLATFORM_FLAGS     = -march=armv7-a
> +*_CLANG35_ARM_PLATFORM_FLAGS     = -march=armv7-a -mkernel -Qunused-arguments

Alternatively, we could switch to armv6 code generation here (except
for the .S pieces which us Thumb2 instructions), which also gets rid
of the movw/movt pairs.

>  *_CLANG35_ARM_PP_FLAGS           = DEF(GCC_PP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
>  *_CLANG35_ARM_RC_FLAGS           = DEF(GCC_ARM_RC_FLAGS)
>  *_CLANG35_ARM_VFRPP_FLAGS        = DEF(GCC_VFRPP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
> --
> 2.19.2
>

Re: [PATCH 2/3] BaseTools/tools_def ARM CLANG35: work around -mno-movt option name change

[Leif Lindholm]

On Wed, Dec 12, 2018 at 01:30:10PM +0100, Ard Biesheuvel wrote:
> On Wed, 12 Dec 2018 at 11:33, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > PE/COFF only has a very limited id space for runtime relocations, and
> > so it defines only a single relocation for movw/movt instruction pairs,
> > which can be combined to load a 32-bit symbol reference into a register.
> > For this to work as expected, these instructions must always appear in
> > the same order and adjacently, and this is something few compilers take
> > into account, unless they target PE/COFF explicitly (and this is not the
> > case for our ELF based toolchains)
> >
> > For Clang 3.6 and later, we can pass the -mno-movt option to suppress
> > movw/movt pairs entirely, which works around the issue. Unfortunately,
> > for Clang 3.5, the option is called differently (-mllvm -arm-use-movt=0)
> > and mutually incompatible between 3.5 and 3.6.
> >
> > Since it is desirable for the CLANG35 toolchain to be usable on newer
> > versions of Clang as well (given that it is the only non-LTO alternative
> > to CLANG38), let's work around this issue in a way that permits versions
> > 3.5 and newer of Clang to be used with the CLANG35 profile.
> >
> > So pass the -mkernel flag instead (and drop the -mno-unaligned-access
> > in *_CLANG35_ARM_CC_XIPFLAGS which now becomes redundant, and which
> > Clang complains about). This also inhibits movw/movt generation, along
> > with some other changes (e.g., long calls) which do affect code generation
> > but not in a undesirable manner.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  BaseTools/Conf/tools_def.template | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
> > index ac2b95e0f5ba..2ba833e1fb06 100755
> > --- a/BaseTools/Conf/tools_def.template
> > +++ b/BaseTools/Conf/tools_def.template
> > @@ -5249,7 +5249,7 @@ DEFINE CLANG35_AARCH64_CC_FLAGS  = DEF(GCC_AARCH64_CC_FLAGS) DEF(CLANG35_AARCH64
> >  *_CLANG35_ARM_ASM_FLAGS          = DEF(GCC_ASM_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHASM_FLAGS) $(PLATFORM_FLAGS) -Qunused-arguments
> >  *_CLANG35_ARM_DLINK_FLAGS        = DEF(CLANG35_ARM_TARGET) DEF(GCC_ARM_DLINK_FLAGS)
> >  *_CLANG35_ARM_DLINK2_FLAGS       = DEF(GCC_DLINK2_FLAGS_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0x220
> > -*_CLANG35_ARM_PLATFORM_FLAGS     = -march=armv7-a
> > +*_CLANG35_ARM_PLATFORM_FLAGS     = -march=armv7-a -mkernel -Qunused-arguments
> 
> Alternatively, we could switch to armv6 code generation here (except
> for the .S pieces which us Thumb2 instructions), which also gets rid
> of the movw/movt pairs.

Please don't.
For the version Laszlo is happy with:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif

> >  *_CLANG35_ARM_PP_FLAGS           = DEF(GCC_PP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
> >  *_CLANG35_ARM_RC_FLAGS           = DEF(GCC_ARM_RC_FLAGS)
> >  *_CLANG35_ARM_VFRPP_FLAGS        = DEF(GCC_VFRPP_FLAGS) DEF(CLANG35_ARM_TARGET) $(ARCHCC_FLAGS) $(PLATFORM_FLAGS)
> > --
> > 2.19.2
> >

[PATCH 3/3] ArmVirtPkg/PrePi ARM CLANG35: drop incompatible command line option

[Ard Biesheuvel]

Drop the -mno-movt command line option override, which is no longer
needed, and actually incompatible with versions of Clang before 3.6.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/ArmVirtQemuKernel.dsc | 5 -----
 ArmVirtPkg/ArmVirtXen.dsc        | 5 -----
 2 files changed, 10 deletions(-)

diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
index c4324a9e264b..df125cfef646 100644
--- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
+++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
@@ -69,11 +69,6 @@ [LibraryClasses.common.UEFI_DRIVER]
   UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
 
 [BuildOptions.common.EDKII.SEC, BuildOptions.common.EDKII.BASE]
-  # Avoid MOVT/MOVW instruction pairs in code that may end up in the PIE
-  # executable we build for the relocatable PrePi. They are not runtime
-  # relocatable in ELF.
-  *_CLANG35_ARM_CC_FLAGS = -mno-movt
-
   #
   # CLANG38 with LTO support enabled uses the GNU GOLD linker, which insists
   # on emitting GOT based symbol references when running in shared mode, unless
diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
index e083666f54ea..a29d8a4ae717 100644
--- a/ArmVirtPkg/ArmVirtXen.dsc
+++ b/ArmVirtPkg/ArmVirtXen.dsc
@@ -58,11 +58,6 @@ [LibraryClasses.common.UEFI_DRIVER]
   UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
 
 [BuildOptions.common.EDKII.SEC, BuildOptions.common.EDKII.BASE]
-  # Avoid MOVT/MOVW instruction pairs in code that may end up in the PIE
-  # executable we build for the relocatable PrePi. They are not runtime
-  # relocatable in ELF.
-  *_CLANG35_ARM_CC_FLAGS = -mno-movt
-
   #
   # CLANG38 with LTO support enabled uses the GNU GOLD linker, which insists
   # on emitting GOT based symbol references when running in shared mode, unless
-- 
2.19.2

Re: [PATCH 3/3] ArmVirtPkg/PrePi ARM CLANG35: drop incompatible command line option

[Laszlo Ersek]

On 12/12/18 11:33, Ard Biesheuvel wrote:
> Drop the -mno-movt command line option override, which is no longer
> needed, and actually incompatible with versions of Clang before 3.6.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirtQemuKernel.dsc | 5 -----
>  ArmVirtPkg/ArmVirtXen.dsc        | 5 -----
>  2 files changed, 10 deletions(-)
> 
> diff --git a/ArmVirtPkg/ArmVirtQemuKernel.dsc b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> index c4324a9e264b..df125cfef646 100644
> --- a/ArmVirtPkg/ArmVirtQemuKernel.dsc
> +++ b/ArmVirtPkg/ArmVirtQemuKernel.dsc
> @@ -69,11 +69,6 @@ [LibraryClasses.common.UEFI_DRIVER]
>    UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
>  
>  [BuildOptions.common.EDKII.SEC, BuildOptions.common.EDKII.BASE]
> -  # Avoid MOVT/MOVW instruction pairs in code that may end up in the PIE
> -  # executable we build for the relocatable PrePi. They are not runtime
> -  # relocatable in ELF.
> -  *_CLANG35_ARM_CC_FLAGS = -mno-movt
> -
>    #
>    # CLANG38 with LTO support enabled uses the GNU GOLD linker, which insists
>    # on emitting GOT based symbol references when running in shared mode, unless
> diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
> index e083666f54ea..a29d8a4ae717 100644
> --- a/ArmVirtPkg/ArmVirtXen.dsc
> +++ b/ArmVirtPkg/ArmVirtXen.dsc
> @@ -58,11 +58,6 @@ [LibraryClasses.common.UEFI_DRIVER]
>    UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
>  
>  [BuildOptions.common.EDKII.SEC, BuildOptions.common.EDKII.BASE]
> -  # Avoid MOVT/MOVW instruction pairs in code that may end up in the PIE
> -  # executable we build for the relocatable PrePi. They are not runtime
> -  # relocatable in ELF.
> -  *_CLANG35_ARM_CC_FLAGS = -mno-movt
> -
>    #
>    # CLANG38 with LTO support enabled uses the GNU GOLD linker, which insists
>    # on emitting GOT based symbol references when running in shared mode, unless
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 0/3] fixes for CLANG35 on ARM

[Leif Lindholm]

On Wed, Dec 12, 2018 at 11:33:05AM +0100, Ard Biesheuvel wrote:
> Building with Clang 3.5 for ARM may result in build breakage, due to the
> fact that it may emit non-adjacent movw/movt instructions pairs which
> cannot be relocated in PE/COFF. We pass -mno-movt in some places to
> work around a related issue in the relocatable PrePi in ArmVirtPkg, but
> we need to disable movw/movt entirely to really address this issue.
> 
> So first, fix some breakage that results from building with -mlong-calls
> in the optimized BaseMemoryLib code (#1)
> 
> Patch #2 switches to -mkernel, which disables movw/movt generation (and
> enabled -mlong-calls as a side effect)
> 
> Patch #3 removes the now redundant, and incompatible command line
> overrides for the relocatable PrePi.

For 1,3/3:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> 
> Ard Biesheuvel (3):
>   MdePkg/BaseMemoryLibOptDxe ARM: add missing function annotations
>   BaseTools/tools_def ARM CLANG35: work around -mno-movt option name
>     change
>   ArmVirtPkg/PrePi ARM CLANG35: drop incompatible command line option
> 
>  ArmVirtPkg/ArmVirtQemuKernel.dsc                     | 5 -----
>  ArmVirtPkg/ArmVirtXen.dsc                            | 5 -----
>  BaseTools/Conf/tools_def.template                    | 2 +-
>  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S | 1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S  | 1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S     | 1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S      | 5 +++++
>  7 files changed, 9 insertions(+), 11 deletions(-)
> 
> -- 
> 2.19.2
> 

Re: [PATCH 0/3] fixes for CLANG35 on ARM

[Gao, Liming]

Ard:
  I have no comments on this patch. So, CLANG38 has no issue. If so, could you recommend use CLANG38? 

Thanks
Liming
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Wednesday, December 12, 2018 6:33 PM
> To: edk2-devel@lists.01.org
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Laszlo Ersek
> <lersek@redhat.com>
> Subject: [PATCH 0/3] fixes for CLANG35 on ARM
> 
> Building with Clang 3.5 for ARM may result in build breakage, due to the
> fact that it may emit non-adjacent movw/movt instructions pairs which
> cannot be relocated in PE/COFF. We pass -mno-movt in some places to
> work around a related issue in the relocatable PrePi in ArmVirtPkg, but
> we need to disable movw/movt entirely to really address this issue.
> 
> So first, fix some breakage that results from building with -mlong-calls
> in the optimized BaseMemoryLib code (#1)
> 
> Patch #2 switches to -mkernel, which disables movw/movt generation (and
> enabled -mlong-calls as a side effect)
> 
> Patch #3 removes the now redundant, and incompatible command line
> overrides for the relocatable PrePi.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> 
> Ard Biesheuvel (3):
>   MdePkg/BaseMemoryLibOptDxe ARM: add missing function annotations
>   BaseTools/tools_def ARM CLANG35: work around -mno-movt option name
>     change
>   ArmVirtPkg/PrePi ARM CLANG35: drop incompatible command line option
> 
>  ArmVirtPkg/ArmVirtQemuKernel.dsc                     | 5 -----
>  ArmVirtPkg/ArmVirtXen.dsc                            | 5 -----
>  BaseTools/Conf/tools_def.template                    | 2 +-
>  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S | 1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S  | 1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S     | 1 +
>  MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S      | 5 +++++
>  7 files changed, 9 insertions(+), 11 deletions(-)
> 
> --
> 2.19.2

Re: [PATCH 0/3] fixes for CLANG35 on ARM

[Ard Biesheuvel]

On Wed, 12 Dec 2018 at 15:01, Gao, Liming <liming.gao@intel.com> wrote:
>
> Ard:
>   I have no comments on this patch. So, CLANG38 has no issue. If so, could you recommend use CLANG38?
>

Yes, the latest is always preferred. However, since CLANG38 enables
LTO, you need the LLVMgold plugin, which is not shipped for all
versions of Clang by the distros. So it is good to keep CLANG35 as a
fallback.

> > -----Original Message-----
> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > Sent: Wednesday, December 12, 2018 6:33 PM
> > To: edk2-devel@lists.01.org
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Laszlo Ersek
> > <lersek@redhat.com>
> > Subject: [PATCH 0/3] fixes for CLANG35 on ARM
> >
> > Building with Clang 3.5 for ARM may result in build breakage, due to the
> > fact that it may emit non-adjacent movw/movt instructions pairs which
> > cannot be relocated in PE/COFF. We pass -mno-movt in some places to
> > work around a related issue in the relocatable PrePi in ArmVirtPkg, but
> > we need to disable movw/movt entirely to really address this issue.
> >
> > So first, fix some breakage that results from building with -mlong-calls
> > in the optimized BaseMemoryLib code (#1)
> >
> > Patch #2 switches to -mkernel, which disables movw/movt generation (and
> > enabled -mlong-calls as a side effect)
> >
> > Patch #3 removes the now redundant, and incompatible command line
> > overrides for the relocatable PrePi.
> >
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Bob Feng <bob.c.feng@intel.com>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> >
> > Ard Biesheuvel (3):
> >   MdePkg/BaseMemoryLibOptDxe ARM: add missing function annotations
> >   BaseTools/tools_def ARM CLANG35: work around -mno-movt option name
> >     change
> >   ArmVirtPkg/PrePi ARM CLANG35: drop incompatible command line option
> >
> >  ArmVirtPkg/ArmVirtQemuKernel.dsc                     | 5 -----
> >  ArmVirtPkg/ArmVirtXen.dsc                            | 5 -----
> >  BaseTools/Conf/tools_def.template                    | 2 +-
> >  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S | 1 +
> >  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S  | 1 +
> >  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S     | 1 +
> >  MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S      | 5 +++++
> >  7 files changed, 9 insertions(+), 11 deletions(-)
> >
> > --
> > 2.19.2
>

Re: [PATCH 0/3] fixes for CLANG35 on ARM

[Gao, Liming]

Make sense. So, CLANG35 tool chain can also be used by CLANG 3.5 and above version compiler, like GCC49?

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: Wednesday, December 12, 2018 10:03 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2] [PATCH 0/3] fixes for CLANG35 on ARM
> 
> On Wed, 12 Dec 2018 at 15:01, Gao, Liming <liming.gao@intel.com> wrote:
> >
> > Ard:
> >   I have no comments on this patch. So, CLANG38 has no issue. If so, could you recommend use CLANG38?
> >
> 
> Yes, the latest is always preferred. However, since CLANG38 enables
> LTO, you need the LLVMgold plugin, which is not shipped for all
> versions of Clang by the distros. So it is good to keep CLANG35 as a
> fallback.
> 
> > > -----Original Message-----
> > > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > > Sent: Wednesday, December 12, 2018 6:33 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Laszlo Ersek
> > > <lersek@redhat.com>
> > > Subject: [PATCH 0/3] fixes for CLANG35 on ARM
> > >
> > > Building with Clang 3.5 for ARM may result in build breakage, due to the
> > > fact that it may emit non-adjacent movw/movt instructions pairs which
> > > cannot be relocated in PE/COFF. We pass -mno-movt in some places to
> > > work around a related issue in the relocatable PrePi in ArmVirtPkg, but
> > > we need to disable movw/movt entirely to really address this issue.
> > >
> > > So first, fix some breakage that results from building with -mlong-calls
> > > in the optimized BaseMemoryLib code (#1)
> > >
> > > Patch #2 switches to -mkernel, which disables movw/movt generation (and
> > > enabled -mlong-calls as a side effect)
> > >
> > > Patch #3 removes the now redundant, and incompatible command line
> > > overrides for the relocatable PrePi.
> > >
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Liming Gao <liming.gao@intel.com>
> > > Cc: Bob Feng <bob.c.feng@intel.com>
> > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > >
> > > Ard Biesheuvel (3):
> > >   MdePkg/BaseMemoryLibOptDxe ARM: add missing function annotations
> > >   BaseTools/tools_def ARM CLANG35: work around -mno-movt option name
> > >     change
> > >   ArmVirtPkg/PrePi ARM CLANG35: drop incompatible command line option
> > >
> > >  ArmVirtPkg/ArmVirtQemuKernel.dsc                     | 5 -----
> > >  ArmVirtPkg/ArmVirtXen.dsc                            | 5 -----
> > >  BaseTools/Conf/tools_def.template                    | 2 +-
> > >  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S | 1 +
> > >  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S  | 1 +
> > >  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S     | 1 +
> > >  MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S      | 5 +++++
> > >  7 files changed, 9 insertions(+), 11 deletions(-)
> > >
> > > --
> > > 2.19.2
> >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/3] fixes for CLANG35 on ARM

[Ard Biesheuvel]

On Wed, 12 Dec 2018 at 15:19, Gao, Liming <liming.gao@intel.com> wrote:
>
> Make sense. So, CLANG35 tool chain can also be used by CLANG 3.5 and above version compiler, like GCC49?
>

Exactly

> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> > Sent: Wednesday, December 12, 2018 10:03 PM
> > To: Gao, Liming <liming.gao@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>
> > Subject: Re: [edk2] [PATCH 0/3] fixes for CLANG35 on ARM
> >
> > On Wed, 12 Dec 2018 at 15:01, Gao, Liming <liming.gao@intel.com> wrote:
> > >
> > > Ard:
> > >   I have no comments on this patch. So, CLANG38 has no issue. If so, could you recommend use CLANG38?
> > >
> >
> > Yes, the latest is always preferred. However, since CLANG38 enables
> > LTO, you need the LLVMgold plugin, which is not shipped for all
> > versions of Clang by the distros. So it is good to keep CLANG35 as a
> > fallback.
> >
> > > > -----Original Message-----
> > > > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > > > Sent: Wednesday, December 12, 2018 6:33 PM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> > > > <liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Leif Lindholm <leif.lindholm@linaro.org>; Laszlo Ersek
> > > > <lersek@redhat.com>
> > > > Subject: [PATCH 0/3] fixes for CLANG35 on ARM
> > > >
> > > > Building with Clang 3.5 for ARM may result in build breakage, due to the
> > > > fact that it may emit non-adjacent movw/movt instructions pairs which
> > > > cannot be relocated in PE/COFF. We pass -mno-movt in some places to
> > > > work around a related issue in the relocatable PrePi in ArmVirtPkg, but
> > > > we need to disable movw/movt entirely to really address this issue.
> > > >
> > > > So first, fix some breakage that results from building with -mlong-calls
> > > > in the optimized BaseMemoryLib code (#1)
> > > >
> > > > Patch #2 switches to -mkernel, which disables movw/movt generation (and
> > > > enabled -mlong-calls as a side effect)
> > > >
> > > > Patch #3 removes the now redundant, and incompatible command line
> > > > overrides for the relocatable PrePi.
> > > >
> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > Cc: Liming Gao <liming.gao@intel.com>
> > > > Cc: Bob Feng <bob.c.feng@intel.com>
> > > > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > >
> > > > Ard Biesheuvel (3):
> > > >   MdePkg/BaseMemoryLibOptDxe ARM: add missing function annotations
> > > >   BaseTools/tools_def ARM CLANG35: work around -mno-movt option name
> > > >     change
> > > >   ArmVirtPkg/PrePi ARM CLANG35: drop incompatible command line option
> > > >
> > > >  ArmVirtPkg/ArmVirtQemuKernel.dsc                     | 5 -----
> > > >  ArmVirtPkg/ArmVirtXen.dsc                            | 5 -----
> > > >  BaseTools/Conf/tools_def.template                    | 2 +-
> > > >  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareGuid.S | 1 +
> > > >  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CompareMem.S  | 1 +
> > > >  MdePkg/Library/BaseMemoryLibOptDxe/Arm/CopyMem.S     | 1 +
> > > >  MdePkg/Library/BaseMemoryLibOptDxe/Arm/SetMem.S      | 5 +++++
> > > >  7 files changed, 9 insertions(+), 11 deletions(-)
> > > >
> > > > --
> > > > 2.19.2
> > >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/3] fixes for CLANG35 on ARM

[Ard Biesheuvel]

On Wed, 12 Dec 2018 at 15:19, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Wed, 12 Dec 2018 at 15:19, Gao, Liming <liming.gao@intel.com> wrote:
> >
> > Make sense. So, CLANG35 tool chain can also be used by CLANG 3.5 and above version compiler, like GCC49?
> >
>
> Exactly
>

Liming,

Are you happy with the MdePkg and BaseTools changes in this series?

Re: [PATCH 0/3] fixes for CLANG35 on ARM

[Gao, Liming]

Yes. Reviewed-by: Liming Gao <liming.gao@intel.com>

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Thursday, December 13, 2018 6:49 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [edk2] [PATCH 0/3] fixes for CLANG35 on ARM
> 
> On Wed, 12 Dec 2018 at 15:19, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > On Wed, 12 Dec 2018 at 15:19, Gao, Liming <liming.gao@intel.com> wrote:
> > >
> > > Make sense. So, CLANG35 tool chain can also be used by CLANG 3.5 and above version compiler, like GCC49?
> > >
> >
> > Exactly
> >
> 
> Liming,
> 
> Are you happy with the MdePkg and BaseTools changes in this series?

Re: [PATCH 0/3] fixes for CLANG35 on ARM

[Ard Biesheuvel]

On Thu, 13 Dec 2018 at 12:42, Gao, Liming <liming.gao@intel.com> wrote:
>
> Yes. Reviewed-by: Liming Gao <liming.gao@intel.com>
>

Thanks all

Pushed as 580f4539dfbb..36deafb838d0

> > -----Original Message-----
> > From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> > Sent: Thursday, December 13, 2018 6:49 PM
> > To: Gao, Liming <liming.gao@intel.com>
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org; Laszlo Ersek <lersek@redhat.com>
> > Subject: Re: [edk2] [PATCH 0/3] fixes for CLANG35 on ARM
> >
> > On Wed, 12 Dec 2018 at 15:19, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > >
> > > On Wed, 12 Dec 2018 at 15:19, Gao, Liming <liming.gao@intel.com> wrote:
> > > >
> > > > Make sense. So, CLANG35 tool chain can also be used by CLANG 3.5 and above version compiler, like GCC49?
> > > >
> > >
> > > Exactly
> > >
> >
> > Liming,
> >
> > Are you happy with the MdePkg and BaseTools changes in this series?