回复: [edk2-devel] [PATCH v4 01/10] BaseTools: add BASETOOLS define

["gaoliming" <gaoliming@byosoft.com.cn>]

Gerd, Ard, Marvin:
  I think we all agree that this patch set is valuable to reduce the duplication header files. 

  The current concern is that BASETOOLS macro name. This name matches Edk2 BaseTools directory. If no better name, I would suggest to keep it. 

  And, the second request is to remove #pragma GCC visibility push (hidden) in MdePkg\Include\X64\ProcessorBind.h. It may be not resolved in short time. I don't want this request blocks this patch set. I suggest to separate them, then we can first merge this patch set if no other functional issue. 

Thanks
Liming
> -----邮件原件-----
> 发件人: Ard Biesheuvel <ardb@kernel.org>
> 发送时间: 2023年4月18日 23:50
> 收件人: Gerd Hoffmann <kraxel@redhat.com>
> 抄送: devel@edk2.groups.io; Marvin Häuser <mhaeuser@posteo.de>; Pawel
> Polawski <ppolawsk@redhat.com>; Dongyan Qian
> <qiandongyan@loongson.cn>; Sunil V L <sunilvl@ventanamicro.com>; Baoqi
> Zhang <zhangbaoqi@loongson.cn>; Chao Li <lichao@loongson.cn>; Rebecca
> Cran <rebecca@bsdio.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Zhiguang Liu <zhiguang.liu@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Yuwei Chen <yuwei.chen@intel.com>; Leif
> Lindholm <quic_llindhol@quicinc.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; Daniel Schaefer <git@danielschaefer.me>;
> Bob Feng <bob.c.feng@intel.com>; Oliver Steffen <osteffen@redhat.com>
> 主题: Re: [edk2-devel] [PATCH v4 01/10] BaseTools: add BASETOOLS define
> 
> On Tue, 18 Apr 2023 at 15:20, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > On Tue, Apr 18, 2023 at 01:59:43PM +0200, Ard Biesheuvel wrote:
> > > On Tue, 18 Apr 2023 at 13:52, Gerd Hoffmann <kraxel@redhat.com>
> wrote:
> > > >
> > > > Seems to work fine on fedora 37, even without adding --relax, maybe
> this
> > > > is enabled by default (there is a --no-relax switch after all).  I'll go
> > > > try older distros / compilers / binutils too.
> > > >
> > > > What would be the failure mode?  Errors on ELF -> PE conversion
> because
> > > > a GOT is present?  Or will things break at runtime?
> > > >
> > >
> > > The problem here is that we rely on --emit-relocs to get at the
> > > relocations in the binary, in order to convert the absolute ones into
> > > PE/COFF relocations.
> > >
> > > However, --emit-relocs did not use to cover the GOT, as those are
> > > added at the end by the linker and not by the compiler. So if the GOT
> > > is non-empty, the resulting PE executable will be corrupt.
> >
> > So no build error.  And at runtime probably random effects, depending on
> > whenever the execution path happens to hit a bad relocation or not.  So
> > compile + boot testing doesn't cut it.  Lovely.
> >
> > So, what can I do instead?  Check **/DEBUG/*.debug and see whenever a
> > .got section is present?
> >
> 
> We should probably add an ASSERT() to the linker script, just like I
> did for Linux:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id
> =e544ea57ac0734bc
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id
> =262b5cae67a67240
> 
> > > Of course, the answer here is to dump GenFw altogether for ELF to PE
> > > conversion, and implement something that consumes the dynamic
> > > relocations generated when linking in PIE mode.
> >
> > Marvin's ImageTool is exactly that I think ...
> >
> > take care,
> >   Gerd
> >