From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by sourceware.org (Postfix) with ESMTP id 2256F3857C7A for ; Thu, 28 Jan 2021 23:25:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2256F3857C7A Received: from [192.168.254.32] (unknown [47.187.219.45]) by linux.microsoft.com (Postfix) with ESMTPSA id 85DB420B7192; Thu, 28 Jan 2021 15:25:04 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 85DB420B7192 Subject: Re: [RFC PATCH v3 2/5] x86: Support for Static Trampolines To: DJ Delorie Cc: libffi-discuss@sourceware.org References: From: "Madhavan T. Venkataraman" Message-ID: <78a00ccd-3724-2a2d-3c59-f4a2b7a309cd@linux.microsoft.com> Date: Thu, 28 Jan 2021 17:25:03 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-20.8 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, ENV_AND_HDR_SPF_MATCH, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libffi-discuss@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libffi-discuss mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Jan 2021 23:25:07 -0000 On 1/28/21 4:17 PM, DJ Delorie wrote: > "Madhavan T. Venkataraman" writes: >>> Extern, but local to this port, yes? >> >> Yes. So, is this declaration acceptable? > > Yup! Great. > >>>> + /* Initialize the dynamic trampoline. */ >>> >>> Should these new APIs be inside #if FFI_EXEC_STATIC_TRAMP ? >>> >> >> Strictly speaking, these should be inside that ifdef. I did it this >> way to avoid too many ifdefs in the code. If you prefer I put them >> inside the ifdefs, I will do it. I will try to minimize the number of >> ifdefs somehow. > > No, it's ok, I was just worried that if ffi_tramp_is_present was in an > #ifdef any callers would be too - but as you noted in another email, > there's always a ffi_tramp_is_present even if it always returns false. > ok. >>> This hack to detect CET should be replaced by the logic in ffitarget.h, >>> or add a #define CET_ENABLED to ffitarget.h >>> >> >> So, _CET_ENDBR for x64 is either defined as: >> >> If CET is present: >> #define _CET_ENDBR endbr64 >> Otherwise: >> #define _CET_ENDBR >> >> So, it is always defined. So, I cannot do something like: > > I was thinking of the conditionals in src/x86/ffitarget.h: > > #if !defined(GENERATE_LIBFFI_MAP) && defined(__ASSEMBLER__) \ > && defined(__CET__) > > Obviously you'd omit the __ASSEMBLER__ one ;-) So, the definition is like this: #if !defined(GENERATE_LIBFFI_MAP) && defined(__ASSEMBLER__) \ && defined(__CET__) # include # define _CET_NOTRACK notrack #else # define _CET_ENDBR # define _CET_NOTRACK #endif Where we include cet.h, CET_ENDBR may still be defined either as endbr64 or empty depending on __CET__ & 1 being non-zero or zero, right? So, would something like this work? #if !defined(GENERATE_LIBFFI_MAP) && defined(__CET__) # include # if (__CET__ & 1) != 0 # define ENDBR_PRESENT # endif # define _CET_NOTRACK notrack #else # define _CET_ENDBR # define _CET_NOTRACK #endif Then, I can do: #ifdef ENDBR_PRESENT blah blah > > If you duplicate the ffitarget.h logic you get the same results. > >>> Copies first argument to %r10, discards return address and arg - closure >>> will return to whoever called it's caller. I'm not sure how this works, >>> which means *at least* a comment needs to be here ;-) >>> >> The target code in this case is the alt entry point. >> >> This is what the alt code is doing: >> - load the data (closure) address in r10 >> - discard the original value of r10 saved on the stack >> since we are using r10 we don't need its original value >> - restore the stack back to what it was when the static trampoline was >> invoked. > > These kinds of short notes should be useful inline comments in the assembler: > > movq 8(%rsp), %r10 /* load closure */ > I will add this. >> I could add a small comment saying "see comment above trampoline_code_table". >> Is that acceptable? > > That would be good too :-) > I will add this as well. Thanks! Madhavan