From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from terminus.zytor.com (terminus.zytor.com [198.137.202.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 41ygtR0QYyzDqgZ for ; Sun, 26 Aug 2018 13:46:38 +1000 (AEST) Subject: Re: [PATCH] treewide: remove current_text_addr To: Helge Deller , Nick Desaulniers , torvalds@linux-foundation.org, akpm@linux-foundation.org Cc: ebiederm@xmission.com, tglx@linutronix.de, mingo@redhat.com, horms@verge.net.au, natechancellor@gmail.com, pombredanne@nexb.com, kstewart@linuxfoundation.org, gregkh@linuxfoundation.org, Richard Henderson , Ivan Kokshaysky , Matt Turner , Vineet Gupta , Russell King , Catalin Marinas , Will Deacon , Mark Salter , Aurelien Jacquiot , Yoshinori Sato , Richard Kuo , Tony Luck , Fenghua Yu , Geert Uytterhoeven , Michal Simek , Ralf Baechle , Paul Burton , James Hogan , Greentime Hu , Vincent Chen , Ley Foon Tan , Jonas Bonn , Stefan Kristiansson , Stafford Horne , "James E.J. Bottomley" , Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , Palmer Dabbelt , Albert Ou , Martin Schwidefsky , Heiko Carstens , Rich Felker , "David S. Miller" , Guan Xuetao , x86@kernel.org, Jeff Dike , Richard Weinberger , Chris Zankel , Max Filippov , Tobias Klauser , Noam Camus , Mickael GUENE , Nicolas Pitre , Kees Cook , Dave Martin , Marc Zyngier , =?UTF-8?Q?Alex_Benn=c3=a9e?= , Laura Abbott , Yury Norov , Mark Rutland , Huacai Chen , "Maciej W. Rozycki" , Arnd Bergmann , David Howells , Sukadev Bhattiprolu , Nicholas Piggin , "Aneesh Kumar K.V" , Philippe Bergheaud , Ram Pai , Christophe Leroy , Cornelia Huck , Vasily Gorbik , Nick Alcock , Shannon Nelson , Nagarathnam Muthusamy , Andy Lutomirski , Borislav Petkov , Dave Hansen , Vitaly Kuznetsov , Jiri Kosina , linux-alpha@vger.kernel.org, linux-kernel@vger.kernel.org, linux-snps-arc@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-c6x-dev@linux-c6x.org, uclinux-h8-devel@lists.sourceforge.jp, linux-hexagon@vger.kernel.org, linux-ia64@vger.kernel.org, linux-m68k@vger.kernel.org, linux-mips@linux-mips.org, nios2-dev@lists.rocketboards.org, openrisc@lists.librecores.org, linux-parisc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-riscv@lists.infradead.org, linux-s390@vger.kernel.org, linux-sh@vger.kernel.org, sparclinux@vger.kernel.org, linux-um@lists.infradead.org References: <20180821202900.208417-1-ndesaulniers@google.com> <207784db-4fcc-85e7-a0b2-fec26b7dab81@gmx.de> From: "H. Peter Anvin" Message-ID: Date: Sat, 25 Aug 2018 19:38:47 -0700 MIME-Version: 1.0 In-Reply-To: <207784db-4fcc-85e7-a0b2-fec26b7dab81@gmx.de> Content-Type: multipart/mixed; boundary="------------CE37549B542F2DC07673DCF7" List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , This is a multi-part message in MIME format. --------------CE37549B542F2DC07673DCF7 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 08/25/18 03:48, Helge Deller wrote: > > Currently alpha, s390, sparc, sh, c6x, ia64 and parisc provide an > inline assembly function to get the current instruction pointer. > As mentioned in an earlier thread, I personally would *prefer* if > _THIS_IP_ would use those inline assembly instructions on those > architectures instead of the (currently used) higher C-level > implementation. > Older ones have as well, e.g. x86. The only reason to retain the use of an assembly function would be in the case where either: a) the C implementation produces bad or invalid code on certain architectures; b) there is a specific requirement that either an absolute or a relative value is used in the binary, e.g. due to constraints on relocation. The latter particularly comes to mind since the x86-64 implementation in assembly will produce movq $.,%reg (which requires relocation) instead of the more natural leaq .(%rip),%reg. In the case (a) those architectures ought to be able to simply #undef _THIS_IP_ #define _THIS_IP_ blah... and in case (b) *those specific instances* should be using some kind of specially flagged function e.g. current_true_ip() vs. current_linktime_ip() or somesuch. I also note that a lot of those functions are not marked __always_inline, which is a serious error should the compiler ever get the idea to out-of-line these functions, which could potentially happen as gcc is rather bad at assigning weight to an assembly statement. I'm also going to throw in a perhaps ugly bomb into this discussion: _THIS_IP_ seems to be horribly ill-defined; there is no kind of serialization, and no reason to believe it can't be arbitrarily hoisted inside a function. Furthermore, *most of the uses of _THIS_IP_ seem to be either discarded or passed to a function*, and the location of a function call, unlike _THIS_IP_ is very well defined. In that case, the use of this mechanism is completely pointless and ought to be replaced with _RET_IP_. It seems like most invocations of _THIS_IP_ can be trivially replaced with _RET_IP_ inside the function, which would also reduce the footprint of the function call, for example: __trace_puts() is only ever called with _THIS_IP_ as the first argument; drop that argument and use _RET_IP_ inside the function (also, __trace_puts() only ever uses strlen() as the third argument, which gcc can of course optimize into a constant for the case of a consta t string, but *is that optimization actually worth it*? In the case of __trace_puts(), a variable strlen() would only ever need to be called in the case of an allocation actually happening -- otherwise str is never examined -- and again, it increases the *code size* of the call site. If it was worthwhile it would make more sense to at least force this into the rodata section with the string, something like the attached file for an example; however, I have a hunch it doesn't matter. I wouldn't be surprised if all or nearly all instances of _THIS_IP_ can be completely removed. -hpa --------------CE37549B542F2DC07673DCF7 Content-Type: text/x-csrc; name="str.c" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="str.c" #include #include #define _RET_IP_ ((unsigned long)__builtin_return_address(0)) #define no_inline __attribute__((noinline)) #define must_inline __attribute__((always_inline)) inline struct myputs_string { size_t len; const char *str; }; int _myputs_struct(const struct myputs_string * const strs); int _myputs_string(const char *str); int __myputs(unsigned long ip, const char *str, size_t len); int no_inline _myputs_struct(const struct myputs_string * const strs) { return __myputs(_RET_IP_, strs->str, strs->len); } int no_inline _myputs_string(const char *str) { return __myputs(_RET_IP_, str, strlen(str)+1); } #define myputs(s) \ ({ \ int _rv; \ if (__builtin_constant_p(s) && \ __builtin_constant_p(strlen(s)) && \ strlen(s)+1 == sizeof(s)) { \ static const struct myputs_string _mps = { \ .len = sizeof(s), \ .str = __builtin_constant_p(s) ? s : NULL, \ }; \ _rv = _myputs_struct(&_mps); \ } else { \ _rv = _myputs_string(s); \ } \ _rv; \ }) int test1(void); int test2(const char *strx); int test1(void) { return myputs("Foobar"); } int test2(const char *strx) { return myputs(strx); } --------------CE37549B542F2DC07673DCF7--