Bug 1540244 - edk2: Does not use RPM build flags
Summary: edk2: Does not use RPM build flags
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: edk2
Version: 28
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Paolo Bonzini
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: Fedora28BuildFlags 1607906
TreeView+ depends on / blocked
 
Reported: 2018-01-30 15:20 UTC by Florian Weimer
Modified: 2018-09-27 17:27 UTC (History)
8 users (show)

Fixed In Version: edk2-20180815gitcb5f4f45ce-1.fc28
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
: 1607906 (view as bug list)
Environment:
Last Closed: 2018-09-27 17:27:39 UTC


Attachments (Terms of Use)
edk2-tools-20180529gitee3198e672e2-4.rhbz1540244.3.fc28.x86_64.log (32.43 KB, text/plain)
2018-08-22 19:41 UTC, Florian Weimer
no flags Details

Description Florian Weimer 2018-01-30 15:20:45 UTC
It seems that during the build of edk2-20171011git92d07e4-3.fc28, no attempt was made to inject the RPM build flags.

BootSectImage is installed as /usr/bin/BootSectImage, so I expect it to be build with the usual complement of compiler/ELF linker flags.  Yet it is compiled with the RPM build flags:

make -C BootSectImage
make[2]: Entering directory '/builddir/build/BUILD/tianocore-edk2-92d07e4/BaseTools/Source/C/BootSectImage'
gcc  -c -MD -fshort-wchar -fno-strict-aliasing -Wall -Werror -Wno-deprecated-declarations -Wno-unused-result -nostdlib -c -g  -I .. -I ../Include/Common -I ../Include/ -I ../Include/IndustryStandard -I ../Common/ -I .. -I . -I ../Include/X64/  -O2 bootsectimage.c -o bootsectimage.o
gcc -o ../bin/BootSectImage  bootsectimage.o -L../libs -lCommon
make[2]: Leaving directory '/builddir/build/BUILD/tianocore-edk2-92d07e4/BaseTools/Source/C/BootSectImage'

Comment 1 Fedora End Of Life 2018-02-20 15:34:16 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 28 development cycle.
Changing version to '28'.

Comment 2 Igor Gnatenko 2018-07-21 07:36:02 UTC
ping?

Comment 5 Laszlo Ersek 2018-07-24 14:39:59 UTC
Thanks for the CC -- I don't understand the report. I understand that the compiler options aren't right, but the details are unclear:

- "usual complement of compiler/ELF linker flags"

vs.

- "compiled with the RPM build flags"

I would expect the latter to mean exactly the former; i.e. I'd expect RPM to inject the necessary flags (and then the BZ would mean that the spec file fails to use the RPM-exposed build options).

Florian, did you mean that the edk2-tools package was built *without* the RPM build flags?

(Anyway I'll definitely follow Paolo's lead here, so please excuse me if the report is clear and my understanding is lacking.)

Comment 6 Laszlo Ersek 2018-07-24 14:43:03 UTC
BTW, external compiler options should not be injected for building the firmware binaries themselves (the edk2-ovmf and edk2-aarch64 noarch packages) -- those builds take very quirky sets of compiler options that upstream manages closely. (The argument "the firmware binaries follow a different ABI" applies.)

Regarding the userspace ELF applications in edk2-tools, I believe that the bug report may be valid.

Comment 7 Daniel Berrangé 2018-07-24 17:31:01 UTC
(In reply to Laszlo Ersek from comment #5)
> Thanks for the CC -- I don't understand the report. I understand that the
> compiler options aren't right, but the details are unclear:
> 
> - "usual complement of compiler/ELF linker flags"
> 
> vs.
> 
> - "compiled with the RPM build flags"
> 
> I would expect the latter to mean exactly the former; i.e. I'd expect RPM to
> inject the necessary flags (and then the BZ would mean that the spec file
> fails to use the RPM-exposed build options).
> 
> Florian, did you mean that the edk2-tools package was built *without* the
> RPM build flags?

There's two things to deal with

When *compiling* native binaries we need to honour the flags in %{optflags}. On F28 that expands to

-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection



When *linking* native binaries we need to honour the flags in %{__global_ldflags}. On Fedora 28 that expands to

-Wl,-z,relro  -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld


NB, the spec should use the %{...} macros not hardcode the flags i illustrate here.

EDKs build system is a custom job so its a matter of figuring out how to pass these options into it.

For the actual firmware builds, as you say, there's quite specific needs that it has to avoid breakage. We should none the less look at each of the flags above and determine which, if any, can be practically & usefully enabled.

Comment 8 Laszlo Ersek 2018-07-24 20:32:32 UTC
(quoting slightly out of order)

(In reply to Daniel Berrange from comment #7)
> For the actual firmware builds, as you say, there's quite specific needs
> that it has to avoid breakage. We should none the less look at each of the
> flags above and determine which, if any, can be practically & usefully
> enabled.

We build all firmware binaries for the DEBUG build target. This target
preserves assertions and debug messages, and does not, as a rule, forbid
compiler optimizations. (Optimizations may be disabled nonetheless, DEBUG
just means that the user doesn't specifically request disabling compiler
optimizations. That would be the NOOPT build target.)

In addition, in Fedora we still use the GCC49 toolchain settings, AFAICS. We
could as well select GCC5 (the upstream default / recommendation is GCC5,
AIUI). The main difference with GCC5 is link time optimization (LTO).

Checking the flags/build logs for DEBUG, and GCC49 / GCC5, yields:

> %{optflags}. On F28 that expands to
>
> -O2

       ARM  AARCH64  IA32  X64
GCC49  -O0  -O0      -Os   -Os
GCC5   -Os  -Os      -Os   -Os

Optimizing for size in firmware binaries makes sense.

In the GCC49/{ARM,AARCH64} cases, -O0 goes back to commit 4afd3d042215
("Sync BaseTool trunk (version r2599) into EDKII BaseTools.", 2013-08-23).
At that time, the upstream edk2 project's source control practices weren't
really advanced, so I can't tell why -O0 was picked.

Either way, the solution for this should be to go with the GCC5 toolchain
settings down-stream. (Tweaking compiler flags for old toolchains won't be
much welcomed up-stream.)

> -g

Present in all six cases. (Note that actual live debugging the firmware,
using the GDB server in QEMU, and debug symbols, takes very awkward measures
that are not upstream. The procedure also does not cover all parts of the
firmware. So extracting debug symbols for RPMs is not useful; the "-g" here
is just a "best effort".)

> -pipe

Does not affect the produced object code.

> -Wall

Present in all six cases.

A very carefully curated set of -Wno-xxx flags is employed, dependent on
toolchain and architecture. I wouldn't like to track down each and every one
of those, but the general practice has been for years that each such
addition is posted and reviewed as a separate patch, with concrete
justification. Examples are:

  -Wno-array-bounds
  -Wno-address
  -Wno-unused-but-set-variable
  -Wno-unused-const-variable

Usually these address false positives that are not possible to suppress or
work around from the source code.

> -Werror=format-security

"-Werror" in itself is present in all six cases.

"format-security" wouldn't be helpful because (a) the firmware isn't built
against a standard C library, (b) while there are printf-like facilities,
their format specifications don't closely follow ISO C.

(QEMU uses GCC_FMT_ATTR(), as a "printf format attribute" wrapper; in edk2,
such isn't currently used, and I wonder if it would be possible at all.)

> -Wp,-D_FORTIFY_SOURCE=2

According to feature_test_macros(7), this instruments some glibc functions
and some gcc intrinsics to catch some buffer overflows. The firmware isn't
built against a C library, and compiler intrinsics are specifically
forbidden / disabled.

(I recall that some ARM/AARCH64 intrinsics were impossible to disable, so
they have been reimplemented / open-coded in the edk2 tree. Those can't be
instrumented either way.)

> -Wp,-D_GLIBCXX_ASSERTIONS

According to the libstdc++ manual, "enables extra error checking in the form
of precondition assertions, such as bounds checking in strings and null
pointer checks when dereferencing smart pointers".

C++ is not used or allowed in edk2.

> -fexceptions

Ditto.

> -fstack-protector-strong

We've got an interesting picture here:

       ARM                AARCH64  IA32                  X64
GCC49  -fstack-protector  [none]   -fno-stack-protector  -fno-stack-protector
GCC5   -fstack-protector  [none]   -fno-stack-protector  -fno-stack-protector

The IA32 and X64 switches go back to historical commit a709adfaf0be ("Sync
tool code to BuildTools project r1783.", 2009-12-29), which is when the GCC
toolchain was enabled in the first place (starting with GCC44).

The ARM setting comes from commit cb6032832355 ("BaseTools: Added support
for GCC stack protector for ARM architecture", 2014-08-20). This suggests
that supporting a stack protector in the firmware requires extra work.

The AARCH64 case (lack of any relevant option) likely signals that noone has
implemented the stack protector for that architecture yet.

> -grecord-gcc-switches

Absent in all six cases. According to the gcc manual, this saves GCC
switches as part of the DWARF debugging information. edk2 uses PE/COFF
though, so even if we wanted to save the compiler switches somehow, this
likely wouldn't be appropriate.

> -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1

I can't really parse this file, but it appears to relate to position
independent code / executables.

       ARM                AARCH64            IA32    X64
GCC49  -fno-pic -fno-pie  -fno-pic -fno-pie  [none]  -fpie
GCC5   -fno-pic -fno-pie  -fno-pic -fno-pie  [none]  -fpie

I recall repeated discussions and reworkings of these flags up-stream
(including code model switches); so I won't even try to track down the list
of commits that have led to the above situation. We definitely cannot change
these flags down-stream.

> -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1

After consulting <https://fedoraproject.org/wiki/Changes/Annobin>; this is
not supported by edk2. If we enabled it, it would likely break the
ELF-to-PE/COFF conversion tools. Intel seems to run static analysis tools on
the edk2 codebase on-and-off, but I don't know any details.

> -m64
> -mtune=generic

These will require a larger table. GCC49 and GCC5 specify the same:

ARM                AARCH64         IA32           X64
-mthumb            -mlittle-endian -m32           -m64
-march=armv7-a     -mcmodel=small  -march=i586    -maccumulate-outgoing-args
-mlittle-endian                    -malign-double -mno-red-zone
-mabi=aapcs                        -mno-mmx       -mcmodel=small
-mfloat-abi=soft                   -mno-sse       -mno-mmx
-mword-relocations                                -mno-sse

I don't think we can change anything about these down-stream.

> -fasynchronous-unwind-tables

All six cases specify the opposite, that is,
"-fno-asynchronous-unwind-tables".

The ".eh_frame" ELF section is not handled by the ELF-to-PE/COFF conversion
tools in edk2; for more background, please see e.g. commit cbf00651eda6
("BaseTools/tools_def: add "-fno-unwind-tables" to GCC_AARCH64_CC_FLAGS",
2018-05-23).

> -fstack-clash-protection

Not used in either of the six cases.

This option is new in gcc-8, and as thus it would require introducing a
"GCC8" toolchain in edk2. Even in that case, the option seems to require
dedicated target / OS support, which edk2 most likely doesn't have.

(The edk2 core provides a feature called "guard pages", however at this
point it is qualified as a debug feature, not a production one. Indeed it
has produced a good number of regressions up-stream, sometimes even for
firmware platforms that don't enable the feature. Thus, none of the six
platforms under discussion enable it, up- or down-stream.)

> -fcf-protection

Again, new in gcc-8. Based on the manual and the release notes, I'm pretty
sure it would take dedicated work to support this in firmware code (if it
wasn't ruled out at once).

> %{__global_ldflags}. On Fedora 28 that expands to
>
> -Wl,-z,relro
> -Wl,-z,now

To my understanding, these together allow ld-linux.so to map various
sections of the executable as read-only into memory. In the firmware, no
dynamic ELF loader like ld-linux.so is used.

> -specs=/usr/lib/rpm/redhat/redhat-hardened-ld

Seems to accompany the compile-time pie/pic flags; indeed this is handled
explicitly in the firmware together with those, wherever appropriate. If one
searches the edk2 git log for "-pie", there are tons of matches.

--*--

Regarding the build tools:

> EDKs build system is a custom job so its a matter of figuring out how to
> pass these options into it.

It requires patching "BaseTools/Source/C/Makefiles/header.makefile".

Comment 9 Laszlo Ersek 2018-07-24 20:38:29 UTC
Wow. I managed to mistype "all *eight* cases" as "all six cases" several times. I'm sorry; I meant "all *eight* cases" every time, corresponding to

  { GCC49, GCC5 } x { ARM, AARCH64, IA32, X64 }

Comment 10 Laszlo Ersek 2018-07-24 21:50:34 UTC
(In reply to Laszlo Ersek from comment #8)

> (The edk2 core provides a feature called "guard pages", however at this
> point it is qualified as a debug feature, not a production one. Indeed it
> has produced a good number of regressions up-stream, sometimes even for
> firmware platforms that don't enable the feature. Thus, none of the six
> platforms under discussion enable it, up- or down-stream.)

* To elaborate a bit, the above applies to the following features from
  "MdeModulePkg.dec":

  - UEFI heap page guard (bit#0 in PcdHeapGuardPropertyMask)
  - UEFI heap pool guard (bit#1)
  - SMM heap page guard (bit#2)
  - SMM heap pool guard (bit#3)
  - UEFI Stack Guard (bool-valued PcdCpuStackGuard)

  Beyond the robustness aspects, the SMM heap guard is known to conflict
  with building static page tables in SMM (see "PcdCpuSmmStaticPageTable" in
  "UefiCpuPkg.dec", and commit 1015fb3c1beb), which we do enable, both
  up-stream and down-stream.

  (For some more background on the SMM static page tables, search
  RHBZ#1468526 for "PcdCpuSmmStaticPageTable".)

* The following feature is enabled however (both upstream and downstream),
  from "UefiCpuPkg.dec":

  - SMM Stack Guard (bool-valued PcdCpuSmmStackGuard)

  See commit 509f8425b75d ("UefiCpuPkg: change PcdCpuSmmStackGuard default
  to TRUE", 2016-06-06), and RHBZ#1341733.

Comment 11 Laszlo Ersek 2018-07-26 00:53:44 UTC
Posted the upstream series "[edk2] [PATCH 0/6] BaseTools/Source/C: take EXTRA_OPTFLAGS and EXTRA_LDFLAGS from the caller".

http://mid.mail-archive.com/20180726004415.13381-1-lersek@redhat.com
https://lists.01.org/pipermail/edk2-devel/2018-July/027606.html

Can be tested like this (outside of an RPM spec file), for example:

source edksetup.sh

make -C "$EDK_TOOLS_PATH" \
  EXTRA_OPTFLAGS="$(rpm --eval '%{optflags}')" \
  EXTRA_LDFLAGS="$(rpm --eval '%{__global_ldflags}')"

build -a IA32 -a X64 -p OvmfPkg/OvmfPkgIa32X64.dsc -t GCC5 -b DEBUG -n 4 \
  -D SMM_REQUIRE -D SECURE_BOOT_ENABLE -D TLS_ENABLE -D NETWORK_IP6_ENABLE \
  -D HTTP_BOOT_ENABLE --cmd-len=65536 --hash --genfds-multi-thread

Comment 12 Laszlo Ersek 2018-08-07 13:09:42 UTC
clearing needinfo from comment 2 first

Comment 13 Laszlo Ersek 2018-08-07 14:16:46 UTC
My patches from comment 11 have received a Reviewed-by from one of the
upstream BaseTools maintainers, Liming Gao. However, Liming has a
preference/request that I have to bring back here, to this RHBZ, for
discussion.

For inclusion in the "edk2-tools" subpackage, we build several native (ELF)
userspace executables. They should be built -- compiled, and linked -- with
the appropriate RPM flags, i.e. %{optflags} and %{__global_ldflags},
respectively. However, one of the utilities is a bit special, namely
"VfrCompile". For that, I first need to provide some background.

(

  The UEFI spec documents the "IFR" format, as a part of the huge HII (Human
  Interface Infrastructure) definition. "IFR" stands for "Internal Forms
  Representation". It is a byte-code format for describing "setup dialogs"
  in a compact and platform-independent manner. The IFR blobs are provided
  as separate "resource-like" sections in the UEFI executables. The UEFI
  Setup Browser program interprets them according to the spec
  (platform-independently), but it displays (renders) them in a platform
  *dependent* manner.

  Consider a discrete PCI network interface card, and a discrete PCI video
  card, both coming with UEFI option ROMs. The NIC will let you configure
  various knobs in the "BIOS Setup" (the kind of knob that you might be
  familiar with from "ethtool" in Linux, for example). The video card will
  let you configure -- also in the "BIOS Setup" -- the preferred boot-time
  resolution, refresh rate, preferred output connector, possibly multi-head,
  and so on.

  For this, the UEFI drivers in the oproms of the cards provide their setup
  dialogs represented as IFR blobs. *How* exactly those dialogs are
  displayed is platform-dependent. On a "gaming platform", the platform
  firmware (integrated in the motherboard) will likely provide a graphical
  "BIOS Setup", with bells and whistles, USB mouse support, animations, nice
  colors, basically everything you do *not* need in a "BIOS Setup". Whereas,
  on a "server platform", you'll probably end up with a bare-bones TUI-like
  "BIOS Setup", with no mouse support, etc.

  The point is that the NIC's and the GPU's setup forms will not look and
  feel "out of place" on either platform, and they will let you configure
  the exact same knobs. On both platforms, the "logic" comes from the oprom
  drivers (native executable code + IFR byte code), while the rendering and
  the human interaction come from the platform firmware.

  Now, the IFR bytecode of the UEFI driver can be produced at both build
  time and runtime. Usually a hybrid approach is used: the statically built
  IFR contains the main form layout, with the labels, questions, buttons,
  anchors, and so on, while the C code of the UEFI driver dynamically
  populates some of the fields (such as drop-down lists, radio buttons,
  ...), by inserting dynamically generated opcodes relative to various
  static anchors. Obviously this is extremely messy, and the general
  approach is to build as much of the IFR as possible statically.

  Enter "VfrCompile". Intel developed a *textual* form descriptor language
  called "VFR" -- Visual Forms Representation. Several edk2 drivers have VFR
  files among their source files, for describing their "BIOS Setup" forms.
  At build time, the "VfrCompile" utility turns those files into IFR
  bytecode.

  The "VfrCompile" utility itself comprises three parts:

  - C++ code that is developed manually,

  - a VFR *lexer* whose C++ source code is generated from "VfrParser.dlg",
    with the "dlg" lexer generator from the PCCTS project,

  - a VFR *parser* whose C++ source code is generated from "VfrSyntax.g",
    with the "antlr" parser generator from the PCCTS project.

  The above approach is not exceptional at all (many projects use "flex" and
  "bison" for similar purposes). The difference is that the "dlg" and
  "antlr" lexer and parser generators come from the PCCTS project, and that
  edk2 *bundles* a modified instance of the PCCTS 1.33 source release
  <http://www.antlr2.org/pccts133.html>.

  Therefore, for building "VfrCompile",

  - we first build the "dlg" and "antlr" utilities from the PCCTS source
    code (which is written in C);

  - then we generate C++ source code for "VfrCompile", from "VfrParser.dlg"
    and "VfrSyntax.g", by running the just-built native "dlg" and "antlr"
    executables;

  - finally, "VfrCompile" is built from (partly generated) source, with g++.

)

Now the question is, whether we should apply %{optflags} and
%{__global_ldflags} to the "dlg" and "antlr" utilities themselves.
Importantly, those lexer/parser generators are run only once -- they are
never installed (or even used for the *firmware* builds); we don't include
them in "edk2-tools".

My patch set in comment 11 does apply %{optflags} and %{__global_ldflags} to
the PCCTS source code too. And, in the past, the upstream edk2 project *has*
modified the bundled PCCTS source code (including makefiles and C code),
whenever it was *necessary* to clean up warnings emitted by the compilers of
various Linux distributions. (I compiled a list at
<http://mid.mail-archive.com/c1f5468f-8ac2-79c3-d3fe-fa35a98cdad9@redhat.com>.)
That said, Liming prefers if we limit the patching of the bundled PCCTS
source to the necessary minimum (including makefiles and C code).

Therefore, my question is, if we consider it *necessary* to build the
one-shot, never installed or shipped, "dlg" and "antlr" lexer/parser
generators too, with the Fedora RPM flags. If we consider that necessary,
Liming is OK with me pushing my patches from comment 11. If we think it's
nice to have, but not really necessary, to build those one-shot utilities
with our RPM flags, then Liming prefers we don't touch the bundled PCCTS
source.

Thoughts? Thanks.

Comment 14 Laszlo Ersek 2018-08-08 19:24:52 UTC
(In reply to Laszlo Ersek from comment #13)

> Therefore, my question is, if we consider it *necessary* to build the
> one-shot, never installed or shipped, "dlg" and "antlr" lexer/parser
> generators too, with the Fedora RPM flags.

Florian, given that you reported the BZ, can you please comment?

If I get no feedback on this soon, then I'll pick "least resistance" and rework my upstream patches so that "dlg" and "antlr" are not built with the flags. The upstream maintainer prefers that. Subsequently, further RPM flag-related BZs -- possibly reported against "dlg" and "antlr" build lines, taken from Koji logs -- will be closed as WONTFIX. The time to discuss and decide is now. Thanks.

Comment 15 Florian Weimer 2018-08-08 19:41:25 UTC
(In reply to Laszlo Ersek from comment #13)
> Now the question is, whether we should apply %{optflags} and
> %{__global_ldflags} to the "dlg" and "antlr" utilities themselves.
> Importantly, those lexer/parser generators are run only once -- they are
> never installed (or even used for the *firmware* builds); we don't include
> them in "edk2-tools".

Has antlr a library part?

> Therefore, my question is, if we consider it *necessary* to build the
> one-shot, never installed or shipped, "dlg" and "antlr" lexer/parser
> generators too, with the Fedora RPM flags.

If it's just the lexer/parser generator, and not support libraries used at run time, it is not necessary to use the Fedora build flags for them.

Comment 16 Laszlo Ersek 2018-08-08 20:10:49 UTC
(In reply to Florian Weimer from comment #15)
> (In reply to Laszlo Ersek from comment #13)
> > Now the question is, whether we should apply %{optflags} and
> > %{__global_ldflags} to the "dlg" and "antlr" utilities themselves.
> > Importantly, those lexer/parser generators are run only once -- they are
> > never installed (or even used for the *firmware* builds); we don't include
> > them in "edk2-tools".
> 
> Has antlr a library part?

No, it doesn't.

(1) The VfrCompile utility (which we include in the edk2-tools package) only contains source code generated by antlr and dlg:

# ldd /usr/bin/VfrCompile
        linux-vdso.so.1 (0x00007fff913e5000)
        libstdc++.so.6 => /lib64/libstdc++.so.6 (0x00007f530b273000)
        libm.so.6 => /lib64/libm.so.6 (0x00007f530aedf000)
        libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f530acc7000)
        libc.so.6 => /lib64/libc.so.6 (0x00007f530a908000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f530b605000)

(2) The antlr and dlg generators themselves are also not linked against any particular shared library:

BaseTools/Source/C/VfrCompile/Pccts/antlr/antlr:
        linux-vdso.so.1 (0x00007fff6c5eb000)
        libc.so.6 => /lib64/libc.so.6 (0x00007f25c4331000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f25c46f0000)

BaseTools/Source/C/VfrCompile/Pccts/dlg/dlg:
        linux-vdso.so.1 (0x00007ffe4bb75000)
        libc.so.6 => /lib64/libc.so.6 (0x00007f7178e06000)
        /lib64/ld-linux-x86-64.so.2 (0x00007f71791c5000)

(Not sure which one your question concerns.)

> > Therefore, my question is, if we consider it *necessary* to build the
> > one-shot, never installed or shipped, "dlg" and "antlr" lexer/parser
> > generators too, with the Fedora RPM flags.
> 
> If it's just the lexer/parser generator, and not support libraries used at
> run time, it is not necessary to use the Fedora build flags for them.

Ah, OK, I understand; you meant (1).

So, no, antlr and dlg do not inject any particular shared library dependency to VfrCompile, and correspondingly, edk2-tools does not install or require any PCCTS-related .so.

# rpm -q --requires edk2-tools
/usr/bin/bash
libc.so.6()(64bit)
libc.so.6(GLIBC_2.14)(64bit)
libc.so.6(GLIBC_2.2.5)(64bit)
libc.so.6(GLIBC_2.3)(64bit)
libc.so.6(GLIBC_2.7)(64bit)
libgcc_s.so.1()(64bit)
libgcc_s.so.1(GCC_3.0)(64bit)
libm.so.6()(64bit)
libm.so.6(GLIBC_2.2.5)(64bit)
libstdc++.so.6()(64bit)
libstdc++.so.6(CXXABI_1.3)(64bit)
libstdc++.so.6(CXXABI_1.3.8)(64bit)
libstdc++.so.6(CXXABI_1.3.9)(64bit)
libstdc++.so.6(GLIBCXX_3.4)(64bit)
libstdc++.so.6(GLIBCXX_3.4.9)(64bit)
libuuid.so.1()(64bit)
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1
rtld(GNU_HASH)

I'll go ahead and rework my series to leave the PCCTS build flags alone. Thank you!

Comment 17 Florian Weimer 2018-08-08 22:47:44 UTC
This is not about shared libraries only.  Statically linked objects count as well.  

BaseTools/Source/C/VfrCompile/Pccts/README says this:

6. You need not create a library.  The makefile created by genmk
   assumes that the files are not part of a library. 

   If you wish to create a library from elements of pccts/h:

   If the first letter of the filename is lowercase (uppercase) it is
   related to the code generated using the pccts C mode (C++ mode).
   Some of the .c and .cpp files in the h directory are not meant to
   be placed in a library and will not compile because they are meant
   to be #include in pccts generated files which are grammar specific.

Those files are in the BaseTools/Source/C/VfrCompile/Pccts/h directory.  If object files compiled from those files end up in installed executables, you should compile the objects with the proper Fedora build flags.

/usr/bin/VfrCompile contains this string:

ANTLRParser::ANTLRParser - Demand lookahead not supported in C++ mode

That's from BaseTools/Source/C/VfrCompile/Pccts/h/AParser.cpp, so the files are indeed compiled and installed.  If you use the wrong build flags for them, annocheck failures will be the result.

Comment 18 Laszlo Ersek 2018-08-09 11:04:12 UTC
(all pathnames relative to BaseTools/Source/C/VfrCompile)

Sure, the files of the kind that you mention are:

- Pccts/h/ATokenBuffer.cpp
- Pccts/h/DLexerBase.cpp
- Pccts/h/AParser.cpp

They are compiled to object files by "GNUmakefile". Conceptually, these files are no different from the antlr / dlg *output*, i.e. from the generated files; the only difference is that they are always "generated" the exact same way, by virtue of them being provided statically by the PCCTS distribution. And, importantly, my patch set covers them, even *without* the flags for the one-shot "antlr" and "dlg" utilities.

In other words, the Pccts/h/*.cpp files go into VfrCompile (and not into "antlr" nor "dlg"), therefore Pccts/h/*.cpp will receive the Fedora RPM compile and link flags, regardless of what we do for "antlr" and "dlg".

Thanks!

Comment 19 Laszlo Ersek 2018-08-09 13:24:06 UTC
Posted the upstream series "[edk2] [PATCH v2 0/5] BaseTools/Source/C: take EXTRA_OPTFLAGS and EXTRA_LDFLAGS from the caller".

https://lists.01.org/pipermail/edk2-devel/2018-August/028297.html
http://mid.mail-archive.com/20180809132258.21874-1-lersek@redhat.com

Comment 20 Laszlo Ersek 2018-08-16 18:25:03 UTC
(In reply to Laszlo Ersek from comment #19)
> Posted the upstream series "[edk2] [PATCH v2 0/5] BaseTools/Source/C: take
> EXTRA_OPTFLAGS and EXTRA_LDFLAGS from the caller".
>
> https://lists.01.org/pipermail/edk2-devel/2018-August/028297.html
> http://mid.mail-archive.com/20180809132258.21874-1-lersek@redhat.com

Upstream commits:

  1 67983484a443 BaseTools/footer.makefile: expand BUILD_CFLAGS last for C
                 files too
  2 03252ae287c4 BaseTools/header.makefile: remove "-c" from BUILD_CFLAGS
  3 b8a661702643 BaseTools/Source/C: split "-O2" to BUILD_OPTFLAGS
  4 b0ca5dae78ff BaseTools/Source/C: take EXTRA_OPTFLAGS from the caller
  5 81502cee20ac BaseTools/Source/C: take EXTRA_LDFLAGS from the caller

Comment 21 Laszlo Ersek 2018-08-17 00:14:16 UTC
The f28 branch in dist-git also misses the following upstream commit (independently of this RHBZ, but without it, we can't build edk2 for aarch64 at all): cbf00651eda6 ("BaseTools/tools_def: add "-fno-unwind-tables" to GCC_AARCH64_CC_FLAGS", 2018-05-23).

Comment 23 Laszlo Ersek 2018-08-17 01:13:27 UTC
So this is the spec file patch on top of the f28 dist-git branch (currently
at commit 158dacfc991c, "Fixes for AMD SEV on OVMF_CODE.fd; add Provides for
bundled OpenSSL", 2018-07-23):

[click "Unwrap Comments" near the top first]

> commit dc8cb9975261381a403f25639ed640f15f063384 (HEAD -> extra_flags_rhbz1540244)
> Author: Laszlo Ersek <lersek@redhat.com>
> Date:   Fri Aug 17 01:26:18 2018 +0200
>
>     use RPM build flags for BaseTools (RHBZ#1540244)
>
>     Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>
> diff --git a/edk2.spec b/edk2.spec
> index 094bde94a363..9635043008f6 100644
> --- a/edk2.spec
> +++ b/edk2.spec
> @@ -35,7 +35,7 @@
>
>  Name:           edk2
>  Version:        %{edk2_date}git%{edk2_githash}
> -Release:        4%{dist}
> +Release:        4.rhbz1540244.2%{dist}
>  Summary:        EFI Development Kit II
>
>  Group:          Applications/Emulators
> @@ -78,6 +78,12 @@ Patch0055: 0055-OvmfPkg-QemuFlashFvbServicesRuntimeDxe-mark-Flash-me.patch
>  Patch0056: 0056-OvmfPkg-QemuFlashFvbServicesRuntimeDxe-Do-not-expose.patch
>  Patch0057: 0057-OvmfPkg-QemuFlashFvbServicesRuntimeDxe-Restore-C-bit.patch
>  Patch0058: 0058-MdeModulePkg-Variable-Check-EFI_MEMORY_RUNTIME-attri.patch
> +Patch0059: 0059-BaseTools-tools_def-add-fno-unwind-tables-to-GCC_AARCH64_CC_FLAGS.patch
> +Patch0060: 0060-BaseTools-footer.makefile-expand-BUILD_CFLAGS-last-for-C-files-too.patch
> +Patch0061: 0061-BaseTools-header.makefile-remove-c-from-BUILD_CFLAGS.patch
> +Patch0062: 0062-BaseTools-Source-C-split-O2-to-BUILD_OPTFLAGS.patch
> +Patch0063: 0063-BaseTools-Source-C-take-EXTRA_OPTFLAGS-from-the-caller.patch
> +Patch0064: 0064-BaseTools-Source-C-take-EXTRA_LDFLAGS-from-the-caller.patch
>
>  %if 0%{?cross:1}
>  # Tweak the tools_def to support cross-compiling.
> @@ -279,7 +285,9 @@ ARM_FLAGS="${CC_FLAGS}"
>  ARM_FLAGS="${ARM_FLAGS} -D DEBUG_PRINT_ERROR_LEVEL=0x8040004F"
>
>  unset MAKEFLAGS
> -make -C BaseTools #%{?_smp_mflags}
> +make -C BaseTools %{?_smp_mflags} \
> +  EXTRA_OPTFLAGS="%{optflags}" \
> +  EXTRA_LDFLAGS="%{__global_ldflags}"
>  sed -i -e 's/-Werror//' Conf/tools_def.txt
>
>

Patch 59 is a verbatim pick of upstream cbf00651eda6 (comment 21); the rest
is listed in comment 20 (also verbatim picks).

Patch 59 enables the fw to build for aarch64 at all. The rest is put to use
in the last hunk of the spec file patch.

Note that I also un-commented %{?_smp_mflags} -- BaseTools now builds with
parallel make.

Comment 24 Laszlo Ersek 2018-08-17 01:29:26 UTC
Scratch build with the changes described in comment 23:

  https://koji.fedoraproject.org/koji/taskinfo?taskID=29125708

... I was about to ask Florian to check this, but then I noticed that I had
missed the linker flags for VfrCompile, in the upstream series. :/

Comment 25 Laszlo Ersek 2018-08-17 02:39:20 UTC
Posted
[PATCH] BaseTools/VfrCompile: honor EXTRA_LDFLAGS
https://lists.01.org/pipermail/edk2-devel/2018-August/028714.html
http://mid.mail-archive.com/20180817023802.7474-1-lersek@redhat.com

Comment 26 Laszlo Ersek 2018-08-22 16:45:50 UTC
(In reply to Laszlo Ersek from comment #25)
> Posted
> [PATCH] BaseTools/VfrCompile: honor EXTRA_LDFLAGS
> https://lists.01.org/pipermail/edk2-devel/2018-August/028714.html
> http://mid.mail-archive.com/20180817023802.7474-1-lersek@redhat.com

This is now upstream commit aa4e0df1f0c7 ("BaseTools/VfrCompile: honor EXTRA_LDFLAGS", 2018-08-22).

Comment 27 Laszlo Ersek 2018-08-22 18:09:22 UTC
https://koji.fedoraproject.org/koji/taskinfo?taskID=29230753

Upstream edk2 patches picked into this build, on top of dist-git
158dacfc991c:

  1  cbf00651eda6 BaseTools/tools_def: add "-fno-unwind-tables" to
                  GCC_AARCH64_CC_FLAGS
  2  67983484a443 BaseTools/footer.makefile: expand BUILD_CFLAGS last for C
                  files too
  3  03252ae287c4 BaseTools/header.makefile: remove "-c" from BUILD_CFLAGS
  4  b8a661702643 BaseTools/Source/C: split "-O2" to BUILD_OPTFLAGS
  5  b0ca5dae78ff BaseTools/Source/C: take EXTRA_OPTFLAGS from the caller
  6  81502cee20ac BaseTools/Source/C: take EXTRA_LDFLAGS from the caller
  7  aa4e0df1f0c7 BaseTools/VfrCompile: honor EXTRA_LDFLAGS

Relevant hunks of the spec file diff:

> @@ -78,6 +78,13 @@ Patch0055: 0055-OvmfPkg-QemuFlashFvbServicesRuntimeDxe-mark-Flash-me.patch
>  Patch0056: 0056-OvmfPkg-QemuFlashFvbServicesRuntimeDxe-Do-not-expose.patch
>  Patch0057: 0057-OvmfPkg-QemuFlashFvbServicesRuntimeDxe-Restore-C-bit.patch
>  Patch0058: 0058-MdeModulePkg-Variable-Check-EFI_MEMORY_RUNTIME-attri.patch
> +Patch0059: 0059-BaseTools-tools_def-add-fno-unwind-tables-to-GCC_AARCH64_CC_FLAGS.patch
> +Patch0060: 0060-BaseTools-footer.makefile-expand-BUILD_CFLAGS-last-for-C-files-too.patch
> +Patch0061: 0061-BaseTools-header.makefile-remove-c-from-BUILD_CFLAGS.patch
> +Patch0062: 0062-BaseTools-Source-C-split-O2-to-BUILD_OPTFLAGS.patch
> +Patch0063: 0063-BaseTools-Source-C-take-EXTRA_OPTFLAGS-from-the-caller.patch
> +Patch0064: 0064-BaseTools-Source-C-take-EXTRA_LDFLAGS-from-the-caller.patch
> +Patch0065: 0065-BaseTools-VfrCompile-honor-EXTRA_LDFLAGS.patch
>
>  %if 0%{?cross:1}
>  # Tweak the tools_def to support cross-compiling.
> @@ -279,7 +286,9 @@ ARM_FLAGS="${CC_FLAGS}"
>  ARM_FLAGS="${ARM_FLAGS} -D DEBUG_PRINT_ERROR_LEVEL=0x8040004F"
>
>  unset MAKEFLAGS
> -make -C BaseTools #%{?_smp_mflags}
> +make -C BaseTools %{?_smp_mflags} \
> +  EXTRA_OPTFLAGS="%{optflags}" \
> +  EXTRA_LDFLAGS="%{__global_ldflags}"
>  sed -i -e 's/-Werror//' Conf/tools_def.txt
>
>

Florian, can you please check if this build is OK? Thanks.

Comment 28 Florian Weimer 2018-08-22 19:41:02 UTC
Created attachment 1477959 [details]
edk2-tools-20180529gitee3198e672e2-4.rhbz1540244.3.fc28.x86_64.log

I ran “annocheck -v” on edk2-tools-20180529gitee3198e672e2-4.rhbz1540244.3.fc28.x86_64, after installing the debuginfo package.  Hardening gaps are still being reported.

This looks a tooling issue (that is, false positives).  I will have another look tomorrow.

Comment 29 Florian Weimer 2018-08-23 19:39:45 UTC
Okay, I had missed that these RPMs were built on Fedora 28.  I took the source RPM and built it on rawhide:

$ annocheck --debug-rpm=edk2-tools-debuginfo-20180529gitee3198e672e2-4.rhbz1540244.3.fc30.x86_64.rpm  edk2-tools-20180529gitee3198e672e2-4.rhbz1540244.3.fc30.x86_64.rpm
Hardened: BootSectImage: PASS.
Hardened: Brotli: PASS.
Hardened: DevicePath: PASS.
Hardened: EfiLdrImage: PASS.
Hardened: EfiRom: PASS.
Hardened: GenCrc32: PASS.
Hardened: GenFfs: PASS.
Hardened: GenFv: PASS.
Hardened: GenFw: PASS.
Hardened: GenPage: PASS.
Hardened: GenSec: PASS.
Hardened: GenVtf: PASS.
Hardened: GnuGenBootSector: PASS.
Hardened: LzmaCompress: PASS.
Hardened: Split: PASS.
Hardened: TianoCompress: PASS.
Hardened: VfrCompile: PASS.
Hardened: VolInfo: PASS.

So it should be fixed.  Thanks for working on this!

Comment 30 Laszlo Ersek 2018-08-24 07:27:54 UTC
Thank you Florian for the verification!

Cole, Gerd, Paolo: can one of you guys please submit official Fedora builds for Fedora 28+?

The BZ was filed for Fedora 28, and the necessary changes for Fedora 28 are described in comment 27.

Regarding Fedora 29 and 30 (rawhide), Cole rebased edk2 in those releases to upstream commit cb5f4f45ce1f (the "edk2-stable201808" tag) yesterday, for bug 1609827. That commit contains commit #1 from comment 27, so for Fedora 29 and 30, commits #2 through #7 (plus the spec file changes) from comment 27 are needed.

Thanks!

Comment 31 Fedora Update System 2018-08-31 19:00:14 UTC
edk2-20180815gitcb5f4f45ce-1.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-9403743de2

Comment 32 Fedora Update System 2018-09-02 06:44:54 UTC
edk2-20180815gitcb5f4f45ce-1.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-9403743de2

Comment 33 Fedora Update System 2018-09-27 17:27:39 UTC
edk2-20180815gitcb5f4f45ce-1.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.