Re: [PATCH v2 0/6] Add ARM support for VS2017

[Pete Batard <pete@akeo.ie>]

Hi Liming,

On 2017.12.07 03:02, Gao, Liming wrote:
>    This is the base step to enable VS2017 ARM tool chain. Then, user could use it and build ARM application first.

Yes.

>    Here, I have one question. VS2017 ARM requires 4096 alignment. So, why not add /FILEALIGN:4096 into tools_def.txt as the default linker option for ARM?

I don't think we want to do that on account that this option does not 
need to be specified when building regular applications, and is 
currently only needed when building the QEMU firmware.

So it's probably better to only specify it in the context where it is 
needed, rather than globally, especially as the documentation for 
/FILEALIGN [1] indicates that "By default, the linker does not use a 
fixed alignment size" which would tend to indicate that we might be 
better off letting the compiler decide what it should use on its own.

Especially, forcing /FILEALIGN to 4K everywhere does appear to increase 
the size of the generated binaries. For instance, I'm seeing a RELEASE 
Shell.efi  with a size of 762 KB without /FILEALIGN, vs. 792 KB with 
/FILEALIGN:4096.

Thus, I think we should be conservative with regards to the global 
compiler options we add: if an option only seems to be needed for a 
single module (and especially, if that module has not yet been patched 
and reviewed for compilation), it should probably be specified for that 
module alone.

As such, unless you or other people on this list have a strong opinion 
that /FILEALIGN:4096 should be global, I am currently not planning to 
resubmit a new patch that adds that option.

Please let me know if you still think the patch should be altered.

Regards,

/Pete

[1] https://docs.microsoft.com/en-gb/cpp/build/reference/filealign