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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
