On 12/03/15 13:54, Ard Biesheuvel wrote:
> The default behavior of the GCC compiler is to emit uninitialized
> globals into a COMMON section, where duplicate definitions are merged.
> This may result in unexpected behavior, since global variables appearing
> by the same name in different C files typically do not refer to the same
> conceptual data item.
>
> For instance, the definitions of EFI_EVENT mVirtualAddressChangeEvent in
> that appear in the following files:
>
> CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
>
> will be folded into a single instance of the variable when the latter
> module includes the former library, which can lead to unexpected
> results.
>
> So prevent GCC from emitting variables into the COMMON section, by
> passing -fno-common to the compiler, and discarding the COMMON section
> in the GNU ld linker script.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <[email protected]>
> ---
>
> NOTE: this patch will result in build failures until we fix the code to make
> at least some instances of mVirtualAddressChangeEvent STATIC
>
> BaseTools/Conf/tools_def.template | 2 +-
> BaseTools/Scripts/GccBase.lds | 3 ++-
> 2 files changed, 3 insertions(+), 2 deletions(-)
I don't recall where and how, but recently I encountered the exact same
phenomenon with gcc. At that time I didn't know about the -fno-common
flag (so thank you for mentioning it), but even so, I thought that
this was a standards conformance bug in gcc.
Now, after reading the -fno-common documentation in the gcc manual, I'm
actually *convinced* this is a gcc bug. The manual says on my RHEL-7.2
system (excerpt):
-fno-common
In C code, controls the placement of uninitialized global
variables. Unix C compilers have traditionally permitted
multiple definitions of such variables in different
compilation units by placing the variables in a common
block. This is the behavior specified by -fcommon, and is
the default for GCC on most targets. On the other hand,
this behavior is not required by ISO C, [...]
Let us stop right there -- this behavior is *forbidden* by ISO C,
which can be proved if someone takes the time to decipher the nearly
impenetrable passages on declarations and definitions.
Namely, consider the following program, consisting of two source files:
f1.c:
----
int x;
----
f2.c:
----
int x;
int main(void) { return x; }
----
(1) Clearly, the above excerpt from the gcc manual applies
("uninitialized global variable").
(2) From C89 "6.7.2 External object definitions":
If the declaration of an identifier for an object has file scope
and an initializer, the declaration is an external definition for
the identifier.
A declaration of an identifier for an object that has file scope
without an initializer, and without a storage-class specifier or
with the storage-class specifier *static*, constitutes a /tentative
definition/. If a translation unit contains one or more tentative
definitions for an identifier, and the translation unit contains no
external definition for that identifier, then the behavior is
exactly as if the translation unit contains a file scope
declaration of that identifier, with the composite type as of the
end of the translation unit, with an initialzier equal to 0.
This means that the above program (of two files) is equivalent to:
f1-b.c:
----
int x = 0;
----
f2-b.c:
----
int x = 0;
int main(void) { return x; }
----
Therefore *each* file is ultimately required to behave "as if" it
contains one external definition for "x".
(3) Considering the *linkage* of "x", in each file independently, it is
external. (Note that this is a separate question from the *definition*
being external!) From 6.1.2.2 "Linkages of identifiers":
[...] If the declaration of an identifier for an object has file
scope and no storage-class specifier, its linkage is external.
(4) Linking those two files into a complete program is a violation of
"6.7 External definitions":
[...] If an identifier declared with external linkage is used in an
expression (other than as part of the operand of a *sizeof*
operator), somewhere in the entire program there shall be exactly
one external definition for the identifier [...]
Again, how does the argument go?
- In each file we have exactly one tentative definition, and no
declaration that would be, in its own right, an external definition.
Based on (2), the files must behave exactly as-if there was one
external *definition* for "x" in each.
- Argument (3) explains why "x" has external *linkage*.
- Argument (4) applies to "x" because "x" has external linkage, and is
used in a non-sizeof expression. And the requirement set forth in (4)
is broken by the program because the program is required to behave
exactly as if "x" had two external definitions.
In practical terms:
- If you compile the first version of the program (without any special
options), it links together successfully:
$ gcc -std=c89 -pedantic -Wall -Wextra -o f f1.c f2.c
- If you compile the second (equivalent) version of the program,
linkage fails:
$ gcc -std=c89 -pedantic -Wall -Wextra -o f-b f1-b.c f2-b.c
/tmp/cc8Isbn8.o:(.bss+0x0): multiple definition of `x'
/tmp/cciQUlDz.o:(.bss+0x0): first defined here
collect2: error: ld returned 1 exit status
- The gcc bug is that linking the first version *too* should fail,
without any particular options.
This patch will enforce the right behavior.
Reviewed-by: Laszlo Ersek <[email protected]>
Thanks!
Laszlo
>
> diff --git a/BaseTools/Conf/tools_def.template
> b/BaseTools/Conf/tools_def.template
> index 564d1336650c..2216e747c02d 100644
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -4318,7 +4318,7 @@ NOOPT_DDK3790xASL_IPF_DLINK_FLAGS = /NOLOGO
> /NODEFAULTLIB /LTCG /DLL /OPT:REF
> DEBUG_*_*_OBJCOPY_ADDDEBUGFLAG =
> --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_NAME).debug
> RELEASE_*_*_OBJCOPY_ADDDEBUGFLAG =
>
> -DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar
> -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h
> +DEFINE GCC_ALL_CC_FLAGS = -g -Os -fshort-wchar
> -fno-strict-aliasing -Wall -Werror -Wno-array-bounds -c -include AutoGen.h
> -fno-common
> DEFINE GCC_IA32_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -m32
> -malign-double -freorder-blocks -freorder-blocks-and-partition -O2
> -mno-stack-arg-probe
> DEFINE GCC_X64_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone
> -Wno-address -mno-stack-arg-probe
> DEFINE GCC_IPF_CC_FLAGS = DEF(GCC_ALL_CC_FLAGS)
> -minline-int-divide-min-latency
> diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
> index 4ee6d998532c..32310bc75dcc 100644
> --- a/BaseTools/Scripts/GccBase.lds
> +++ b/BaseTools/Scripts/GccBase.lds
> @@ -46,7 +46,7 @@ SECTIONS {
> */
> .data ALIGN(ALIGNOF(.text)) : ALIGN(CONSTANT(COMMONPAGESIZE)) {
> *(.data .data.* .gnu.linkonce.d.*)
> - *(.bss .bss.* *COM*)
> + *(.bss .bss.*)
> }
>
> .eh_frame ALIGN(CONSTANT(COMMONPAGESIZE)) : {
> @@ -66,5 +66,6 @@ SECTIONS {
> *(.dynamic)
> *(.hash)
> *(.comment)
> + *(COMMON)
> }
> }
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
