Re: [PATCH 0/4] BaseTools: Add VS2017 support, including ARM and AARCH64

[Pete Batard <pete@akeo.ie>]

Hi Liming,

On 2017.11.14 16:03, Gao, Liming wrote:
>   In fact, I have sent one patch serial to add VS2017 tool chain for 
> IA32 and X64. https://lists.01.org/pipermail/edk2-devel/2017-October/016175.html

I missed that proposal, else I would have used it as my base. Thanks for 
pointing it out.

>   In my patch, I disable warning 4701&4703, because they are also disabled in VS2015.

Okay. Then I see no objections to disabling them.

I didn't see the /wd options for those in tools_def.template with 
VS2015, so I thought these needed to be addressed.

But from your looking at your proposal, I realize that these are 
disabled in ProcessorBind.h. Obviously, I'll update my ARM/AARCH64 
proposal so that the warnings for these toolchains are disabled there as 
well, instead of tools_def.template.

> In tools_def.template, to align other VS tool chain, I use VS2017_BIN for 32bit, 
> VS2017_BINX64 for 64bit. Could you follow this rule to define VS2017_BINARM for
> arm, VS2017_BINAARCH64 for AARCH64?

I will do that.

> On VS2017, I notice it doesn't set VS150COMNTOOLS env by default. 
> So, I use vswhere.exe to detect VS2017 installation path. When we build source
> code with VS tool chain, we open normal cmd instead of VS cmd.

I see. I've always been opening a VS command prompt before calling the 
EDK setup script, so I built my proposal around that. But I agree that 
it makes sense to be able to build from regular prompt, so I will try to 
follow your lead.

My only concern is that it may be difficult to locate the relevant ARM 
toolchains without %WindowsSdkVerBinPath% being properly defined.

In your proposal, you seem to be hardcoding WINSDK10_PREFIX to something 
like "%ProgramFiles(x86)%\Windows Kits\10\bin\x86" but that won't work 
with ARM/ARM64 as we will need to get the actual version of the defayult 
SDK installed for VS2017 (e.g. "C:\Program Files (x86)\Windows 
Kits\10\bin\10.0.16299.0\") as the ARM toolchains will not work otherwise.

For instance, on my VS2017 installation the "arm64\" under "Windows 
Kits\10\bin\" does not contain the latest version of the ARM64 tools and 
libraries, which is problematic as proper ARM64 compilation support was 
only enabled with the latest update. Only the one under "Windows 
Kits\10\bin\10.0.16299.0\" does.

All in all, rather than try to guess the SDK path, I think we should try 
to locate and call "%ProgramFiles(x86)%\Microsoft Visual 
Studio\2017\Community\Common7\Tools\vsdevcmd\core\winsdk.bat", to have 
the VS environment provide us with the actual SDK version to use, as 
determined by Microsoft.

> In your patch on ARM support, <stdarg.h> is included in Base.h.
> This is a windows header file. So, VS env must be setup to build source
> code with ARM arch. Right?

That is correct. I had a quick look at removing that header, but didn't 
see much harm in keeping it. However, now that I understand our 
constraints better, I will make sure that header is not referenced.

I will also do the same for memset_ms.c and memcpy_ms.c, introduced for 
ARM/ARM64 (In CompilerIntrinsicsLib), as it references <stddef.h> for 
size_t.

> Last, I suggest you base on my patch to add VS2017 ARM and AARCH64 arch.
 > For your patches, I suggest you separate them per package.

I will follow both these suggestions and send a v2 when ready.

Regards,

/Pete