public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] OvmfPkg/Sec: Fix 64bit SEC build failure
@ 2017-11-27  1:38 Ruiyu Ni
  2017-11-27 15:02 ` Laszlo Ersek
  0 siblings, 1 reply; 4+ messages in thread
From: Ruiyu Ni @ 2017-11-27  1:38 UTC (permalink / raw)
  To: edk2-devel; +Cc: Laszlo Ersek

Original code breaks a single assembly code to multiple lines.
But build tool doesn't support such usage in Windows OS environment.

Changing the multiple lines to one line to resolve the build failure.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Sec/X64/SecEntry.nasm | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/Sec/X64/SecEntry.nasm b/OvmfPkg/Sec/X64/SecEntry.nasm
index 7c55032ac9..d76adcffd8 100644
--- a/OvmfPkg/Sec/X64/SecEntry.nasm
+++ b/OvmfPkg/Sec/X64/SecEntry.nasm
@@ -45,10 +45,8 @@ ASM_PFX(_ModuleEntryPoint):
     ; Fill the temporary RAM with the initial stack value.
     ; The loop below will seed the heap as well, but that's harmless.
     ;
-    mov     rax, (FixedPcdGet32 (                        \
-                    PcdInitValueInTempStack              \
-                    ) << 32) |                           \
-                 FixedPcdGet32 (PcdInitValueInTempStack)      ; qword to store
+    mov     rax, (FixedPcdGet32 (PcdInitValueInTempStack) << 32) | FixedPcdGet32 (PcdInitValueInTempStack)
+                                                              ; qword to store
     mov     rdi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase)     ; base address,
                                                               ;   relative to
                                                               ;   ES
-- 
2.15.0.gvfs.1.preview.4



^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] OvmfPkg/Sec: Fix 64bit SEC build failure
  2017-11-27  1:38 [PATCH] OvmfPkg/Sec: Fix 64bit SEC build failure Ruiyu Ni
@ 2017-11-27 15:02 ` Laszlo Ersek
  2017-11-28 10:14   ` Gao, Liming
  0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2017-11-27 15:02 UTC (permalink / raw)
  To: Ruiyu Ni
  Cc: edk2-devel, Zhu, Yonghong, Gao, Liming, Ard Biesheuvel,
	Jordan Justen (Intel address)

Hi Ray,

adding Ard, Jordan, Liming and Yonghong:

On 11/27/17 02:38, Ruiyu Ni wrote:
> Original code breaks a single assembly code to multiple lines.
> But build tool doesn't support such usage in Windows OS environment.
> 
> Changing the multiple lines to one line to resolve the build failure.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/Sec/X64/SecEntry.nasm | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/OvmfPkg/Sec/X64/SecEntry.nasm b/OvmfPkg/Sec/X64/SecEntry.nasm
> index 7c55032ac9..d76adcffd8 100644
> --- a/OvmfPkg/Sec/X64/SecEntry.nasm
> +++ b/OvmfPkg/Sec/X64/SecEntry.nasm
> @@ -45,10 +45,8 @@ ASM_PFX(_ModuleEntryPoint):
>      ; Fill the temporary RAM with the initial stack value.
>      ; The loop below will seed the heap as well, but that's harmless.
>      ;
> -    mov     rax, (FixedPcdGet32 (                        \
> -                    PcdInitValueInTempStack              \
> -                    ) << 32) |                           \
> -                 FixedPcdGet32 (PcdInitValueInTempStack)      ; qword to store
> +    mov     rax, (FixedPcdGet32 (PcdInitValueInTempStack) << 32) | FixedPcdGet32 (PcdInitValueInTempStack)
> +                                                              ; qword to store
>      mov     rdi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase)     ; base address,
>                                                                ;   relative to
>                                                                ;   ES
> 

I'd like to understand more about the build failure. As far as I understand, the NASM tool itself has no problem with the backslash line continuation; before I posted the patch, I checked the NASM manual for this feature.

http://www.nasm.us/doc/nasmdoc3.html#section-3.1

    NASM uses backslash (\) as the line continuation character; if a
    line ends with backslash, the next line is considered to be a part
    of the backslash-ended line.

Now, in "BaseTools/Conf/build_rule.template", I see that we have the following rule for NASM source files:

[Nasm-Assembly-Code-File.COMMON.COMMON]
    <InputFile>
        ?.nasm

    <ExtraDependency>
        $(MAKE_FILE)

    <OutputFile>
        $(OUTPUT_DIR)(+)${s_dir}(+)${s_base}.obj

    <Command>
        "$(PP)" $(PP_FLAGS) $(INC) ${src} > ${d_path}(+)${s_base}.i
        Trim --trim-long --source-code -o ${d_path}(+)${s_base}.iii ${d_path}(+)${s_base}.i
        "$(NASM)" -I${s_path}(+) $(NASM_FLAGS) -o $dst ${d_path}(+)${s_base}.iii

I guess it's actually the pre-processor (PP) that splices the broken-up lines together, as any C language pre-processor is supposed to do.

Why does that not work with the VS toolchains ("cl.exe")?

Is it perhaps a problem with the _PP_FLAGS?

https://msdn.microsoft.com/en-us/library/3xkfswhy.aspx
https://msdn.microsoft.com/en-us/library/032xwy55.aspx

"/E" seems right (pre-process). Is /TC the problem? ("C language source code").

If the problem is impossible to fix in BaseTools, I don't mind the patch, but I'd like to see more explanation in the commit message.

(Other assembly files use backslash line continuations too, they just haven't been exposed to VS toolchains yet, apparently.)

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] OvmfPkg/Sec: Fix 64bit SEC build failure
  2017-11-27 15:02 ` Laszlo Ersek
@ 2017-11-28 10:14   ` Gao, Liming
  2017-11-28 13:57     ` Laszlo Ersek
  0 siblings, 1 reply; 4+ messages in thread
From: Gao, Liming @ 2017-11-28 10:14 UTC (permalink / raw)
  To: Laszlo Ersek, Ni, Ruiyu
  Cc: edk2-devel@lists.01.org, Zhu, Yonghong, Ard Biesheuvel,
	Justen, Jordan L

This is VS CL issue.  It preprocesses below FixedPcdGet32() to the fixed value, but lost '\'. So, it will cause nasm compiler failure. To avoid it, we need to make sure FixedPcdGet32 (PcdInitValueInTempStack) at one line.

  FixedPcdGet32 (                        \
                    PcdInitValueInTempStack              \
                    )
==> 


0x5AA55AA5U

Thanks
Liming
>-----Original Message-----
>From: Laszlo Ersek [mailto:lersek@redhat.com]
>Sent: Monday, November 27, 2017 11:03 PM
>To: Ni, Ruiyu <ruiyu.ni@intel.com>
>Cc: edk2-devel@lists.01.org; Zhu, Yonghong <yonghong.zhu@intel.com>; Gao,
>Liming <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
>Justen, Jordan L <jordan.l.justen@intel.com>
>Subject: Re: [PATCH] OvmfPkg/Sec: Fix 64bit SEC build failure
>
>Hi Ray,
>
>adding Ard, Jordan, Liming and Yonghong:
>
>On 11/27/17 02:38, Ruiyu Ni wrote:
>> Original code breaks a single assembly code to multiple lines.
>> But build tool doesn't support such usage in Windows OS environment.
>>
>> Changing the multiple lines to one line to resolve the build failure.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  OvmfPkg/Sec/X64/SecEntry.nasm | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/OvmfPkg/Sec/X64/SecEntry.nasm
>b/OvmfPkg/Sec/X64/SecEntry.nasm
>> index 7c55032ac9..d76adcffd8 100644
>> --- a/OvmfPkg/Sec/X64/SecEntry.nasm
>> +++ b/OvmfPkg/Sec/X64/SecEntry.nasm
>> @@ -45,10 +45,8 @@ ASM_PFX(_ModuleEntryPoint):
>>      ; Fill the temporary RAM with the initial stack value.
>>      ; The loop below will seed the heap as well, but that's harmless.
>>      ;
>> -    mov     rax, (FixedPcdGet32 (                        \
>> -                    PcdInitValueInTempStack              \
>> -                    ) << 32) |                           \
>> -                 FixedPcdGet32 (PcdInitValueInTempStack)      ; qword to store
>> +    mov     rax, (FixedPcdGet32 (PcdInitValueInTempStack) << 32) |
>FixedPcdGet32 (PcdInitValueInTempStack)
>> +                                                              ; qword to store
>>      mov     rdi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase)     ; base
>address,
>>                                                                ;   relative to
>>                                                                ;   ES
>>
>
>I'd like to understand more about the build failure. As far as I understand, the
>NASM tool itself has no problem with the backslash line continuation; before I
>posted the patch, I checked the NASM manual for this feature.
>
>http://www.nasm.us/doc/nasmdoc3.html#section-3.1
>
>    NASM uses backslash (\) as the line continuation character; if a
>    line ends with backslash, the next line is considered to be a part
>    of the backslash-ended line.
>
>Now, in "BaseTools/Conf/build_rule.template", I see that we have the
>following rule for NASM source files:
>
>[Nasm-Assembly-Code-File.COMMON.COMMON]
>    <InputFile>
>        ?.nasm
>
>    <ExtraDependency>
>        $(MAKE_FILE)
>
>    <OutputFile>
>        $(OUTPUT_DIR)(+)${s_dir}(+)${s_base}.obj
>
>    <Command>
>        "$(PP)" $(PP_FLAGS) $(INC) ${src} > ${d_path}(+)${s_base}.i
>        Trim --trim-long --source-code -o ${d_path}(+)${s_base}.iii
>${d_path}(+)${s_base}.i
>        "$(NASM)" -I${s_path}(+) $(NASM_FLAGS) -o $dst
>${d_path}(+)${s_base}.iii
>
>I guess it's actually the pre-processor (PP) that splices the broken-up lines
>together, as any C language pre-processor is supposed to do.
>
>Why does that not work with the VS toolchains ("cl.exe")?
>
>Is it perhaps a problem with the _PP_FLAGS?
>
>https://msdn.microsoft.com/en-us/library/3xkfswhy.aspx
>https://msdn.microsoft.com/en-us/library/032xwy55.aspx
>
>"/E" seems right (pre-process). Is /TC the problem? ("C language source
>code").
>
>If the problem is impossible to fix in BaseTools, I don't mind the patch, but I'd
>like to see more explanation in the commit message.
>
>(Other assembly files use backslash line continuations too, they just haven't
>been exposed to VS toolchains yet, apparently.)
>
>Thanks
>Laszlo

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] OvmfPkg/Sec: Fix 64bit SEC build failure
  2017-11-28 10:14   ` Gao, Liming
@ 2017-11-28 13:57     ` Laszlo Ersek
  0 siblings, 0 replies; 4+ messages in thread
From: Laszlo Ersek @ 2017-11-28 13:57 UTC (permalink / raw)
  To: Gao, Liming, Ni, Ruiyu
  Cc: edk2-devel@lists.01.org, Zhu, Yonghong, Ard Biesheuvel,
	Justen, Jordan L

On 11/28/17 11:14, Gao, Liming wrote:
> This is VS CL issue.  It preprocesses below FixedPcdGet32() to the fixed value, but lost '\'. So, it will cause nasm compiler failure. To avoid it, we need to make sure FixedPcdGet32 (PcdInitValueInTempStack) at one line.
> 
>   FixedPcdGet32 (                        \
>                     PcdInitValueInTempStack              \
>                     )
> ==> 
> 
> 
> 0x5AA55AA5U

Thank you.

Ray, please add the above information from Liming to the commit message:

- remove "But build tool doesn't support such usage in Windows OS
environment"

- add "But, when VS CL.exe preprocesses the FixedPcdGet32() macro
invocation to the replacement text, it loses '\', and causes NASM to fail."

With that,

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

Thanks!
Laszlo

> 
> Thanks
> Liming
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, November 27, 2017 11:03 PM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Cc: edk2-devel@lists.01.org; Zhu, Yonghong <yonghong.zhu@intel.com>; Gao,
>> Liming <liming.gao@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
>> Justen, Jordan L <jordan.l.justen@intel.com>
>> Subject: Re: [PATCH] OvmfPkg/Sec: Fix 64bit SEC build failure
>>
>> Hi Ray,
>>
>> adding Ard, Jordan, Liming and Yonghong:
>>
>> On 11/27/17 02:38, Ruiyu Ni wrote:
>>> Original code breaks a single assembly code to multiple lines.
>>> But build tool doesn't support such usage in Windows OS environment.
>>>
>>> Changing the multiple lines to one line to resolve the build failure.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>  OvmfPkg/Sec/X64/SecEntry.nasm | 6 ++----
>>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/OvmfPkg/Sec/X64/SecEntry.nasm
>> b/OvmfPkg/Sec/X64/SecEntry.nasm
>>> index 7c55032ac9..d76adcffd8 100644
>>> --- a/OvmfPkg/Sec/X64/SecEntry.nasm
>>> +++ b/OvmfPkg/Sec/X64/SecEntry.nasm
>>> @@ -45,10 +45,8 @@ ASM_PFX(_ModuleEntryPoint):
>>>      ; Fill the temporary RAM with the initial stack value.
>>>      ; The loop below will seed the heap as well, but that's harmless.
>>>      ;
>>> -    mov     rax, (FixedPcdGet32 (                        \
>>> -                    PcdInitValueInTempStack              \
>>> -                    ) << 32) |                           \
>>> -                 FixedPcdGet32 (PcdInitValueInTempStack)      ; qword to store
>>> +    mov     rax, (FixedPcdGet32 (PcdInitValueInTempStack) << 32) |
>> FixedPcdGet32 (PcdInitValueInTempStack)
>>> +                                                              ; qword to store
>>>      mov     rdi, FixedPcdGet32 (PcdOvmfSecPeiTempRamBase)     ; base
>> address,
>>>                                                                ;   relative to
>>>                                                                ;   ES
>>>
>>
>> I'd like to understand more about the build failure. As far as I understand, the
>> NASM tool itself has no problem with the backslash line continuation; before I
>> posted the patch, I checked the NASM manual for this feature.
>>
>> http://www.nasm.us/doc/nasmdoc3.html#section-3.1
>>
>>    NASM uses backslash (\) as the line continuation character; if a
>>    line ends with backslash, the next line is considered to be a part
>>    of the backslash-ended line.
>>
>> Now, in "BaseTools/Conf/build_rule.template", I see that we have the
>> following rule for NASM source files:
>>
>> [Nasm-Assembly-Code-File.COMMON.COMMON]
>>    <InputFile>
>>        ?.nasm
>>
>>    <ExtraDependency>
>>        $(MAKE_FILE)
>>
>>    <OutputFile>
>>        $(OUTPUT_DIR)(+)${s_dir}(+)${s_base}.obj
>>
>>    <Command>
>>        "$(PP)" $(PP_FLAGS) $(INC) ${src} > ${d_path}(+)${s_base}.i
>>        Trim --trim-long --source-code -o ${d_path}(+)${s_base}.iii
>> ${d_path}(+)${s_base}.i
>>        "$(NASM)" -I${s_path}(+) $(NASM_FLAGS) -o $dst
>> ${d_path}(+)${s_base}.iii
>>
>> I guess it's actually the pre-processor (PP) that splices the broken-up lines
>> together, as any C language pre-processor is supposed to do.
>>
>> Why does that not work with the VS toolchains ("cl.exe")?
>>
>> Is it perhaps a problem with the _PP_FLAGS?
>>
>> https://msdn.microsoft.com/en-us/library/3xkfswhy.aspx
>> https://msdn.microsoft.com/en-us/library/032xwy55.aspx
>>
>> "/E" seems right (pre-process). Is /TC the problem? ("C language source
>> code").
>>
>> If the problem is impossible to fix in BaseTools, I don't mind the patch, but I'd
>> like to see more explanation in the commit message.
>>
>> (Other assembly files use backslash line continuations too, they just haven't
>> been exposed to VS toolchains yet, apparently.)
>>
>> Thanks
>> Laszlo



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2017-11-28 13:53 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-27  1:38 [PATCH] OvmfPkg/Sec: Fix 64bit SEC build failure Ruiyu Ni
2017-11-27 15:02 ` Laszlo Ersek
2017-11-28 10:14   ` Gao, Liming
2017-11-28 13:57     ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox