[PATCH] D72222: [Driver][CodeGen] Add -fpatchable-function-entry=N[,0]

Thu Jan 23 13:43:10 PST 2020

MaskRay added a comment.

In D72222#1836942 <https://reviews.llvm.org/D72222#1836942>, @nickdesaulniers wrote:

> In D72222#1836687 <https://reviews.llvm.org/D72222#1836687>, @MaskRay wrote:
>
> > (I really dislike how the feature was developed on the GCC side. Yet another Linux-kernel specific GCC option when there are already 4 existing options for the same feature)
>
>
> Maybe I'm reading too much into it, but please always have respect for the competition, as a representative of the company and LLVM community you represent, as well as my friend.  It should be clear why that's important and why I won't tolerate anything less.  If you still disagree, I'm happy to talk more about it privately.
>
> Can you enumerate the 4 existing options?  If we do already have an existing tool in our toolbox, it would be good to consider their strengths and limitations before building a new tool.

Apologies if my word felt strong. I meant I felt sad that GCC developed another option in this area, along with -mfentry, -mhotpatch (SystemZ specific), -mnop-mcount (x86, SystemZ, -fno-pie specific), -mrecord-mcount (used to retire `scripts/recordmcount.{pl,c}`). The GCC implementation has several issues, as I listed in the link I pasted above. (I fixed a GCC bug in this area.) I also feel making the dependent Linux features enabled by default when -fpatchable-function-entry= is recognized by the compiler was not sufficient. It should have more sophisticated check.

The idea of this option is great. The intention was to unify the existing options. However, two major flaws (comdat (C++) / --gc-sections) make it impractical for non-Linux-kernel applications. I noticed we recently add support for -mnop-mcount. I haven't made enough investigation, but I suspect we don't have to do that and can use a general option instead.

@hans To address the CI problem (https://reviews.llvm.org/D72222#1836728) reported by @peter.smith, we just need to delete the driver option in the release/10.x branch. (I need a -E preproccessed file to investigate.)
I think we don't need to hurry. We can simply revert this patch (D72222 <https://reviews.llvm.org/D72222>) a few days before 10.0.0.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72222/new/

https://reviews.llvm.org/D72222