xiangzhangllvm added inline comments.
================
Comment at: llvm/lib/Target/X86/X86IndirectBranchTracking.cpp:156-161
if (needsPrologueENDBR(MF, M)) {
- auto MBB = MF.begin();
- Changed |= addENDBR(*MBB, MBB->begin());
+ if (!ManualENDBR || MF.getFunction().doesCfCheck()) {
+ auto MBB = MF.begin();
+ Changed |= addENDBR(*MBB, MBB->begin());
+ } else {
+ // When -mmanual-endbr is in effect, the compiler does not
----------------
joaomoreira wrote:
> xiangzhangllvm wrote:
> > I am a little puzzle here. Let me copy your patch description here:
> >
> > ```
> > >When -mmanual-endbr is set, llvm refrains from automatically adding
> > >ENDBR instructions to functions' prologues, which would have been
> > >automatically added by -fcf-protection=branch. Although this works
> > >correctly, missing ENDBR instructions where they are actually needed
> > >could lead to broken binaries, which would fail only in running time.
> > ```
> > I think the purpose of "-mmanual-endbr" should be "Supplementary Way" for
> > the cases which the CET can't correctly insert endbr in automatic way.
> > Here (in if (needsPrologueENDBR(MF, M)) ) the automatic way will insert the
> > endbr. So I think the job for "-mmanual-endbr" should be done in parallel
> > with the old condition. Some like:
> > ```
> > if (ManualENDBR ){
> > do something
> > }else { // automatic
> > if (needsPrologueENDBR(MF, M)) {insert endbr}
> > }
> > }
> > ```
> I don't think the idea of -mmanual-endbr is to have a supplementary way for
> corner cases where CET misses automatic placement. In my understanding the
> goal is to set the compiler to not emit ENDBRs unless the attribute cf_check
> is used, thus providing a way to manually reduce the number of valid targets.
>
> For reference, here is a link for -mmanual-endbr on gcc,
> https://gcc.gnu.org/legacy-ml/gcc-patches/2018-12/msg00713.html and here are
> patches on xen that use the feature (and that also mention this review):
> https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg114160.html
Thanks! Ok, for "limit number of ENDBR instructions to reduce program size"
here the code logic is make sense.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118355/new/
https://reviews.llvm.org/D118355
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
