From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 5987 invoked from network); 5 Jul 2020 01:36:44 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 5 Jul 2020 01:36:44 -0000 Received: (qmail 8074 invoked by uid 550); 5 Jul 2020 01:36:41 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 8045 invoked from network); 5 Jul 2020 01:36:40 -0000 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=pobox.com; h=subject:to :references:from:cc:message-id:date:mime-version:in-reply-to :content-type:content-transfer-encoding; s=sasl; bh=GG5ujkpt1taL y2wiG9qPadLXUDE=; b=mxmkawctS5MF1S59JPHb5dk9lXv3MdVHUapc1ZqOXT4M cNfSADlcyBWik1GLuL2TMU+sgv/hcqiUdYmZnNhLcx7bn5jI+7DVq9rwSYHEN+g7 is5bxd9hevw3HZ05pRprgt19ofu/ErSVLA3TdsOoYjJIce1EOcbi7jJU/MFrj2A= DomainKey-Signature: a=rsa-sha1; c=nofws; d=pobox.com; h=subject:to :references:from:cc:message-id:date:mime-version:in-reply-to :content-type:content-transfer-encoding; q=dns; s=sasl; b=yTKkUU jERuHlQFrazW5LDeRsFv9HOWU2pChC0UUdh4r4DLUsQOrM54iK9lWhLHXZbPNLpx jeDfHfJH++hxC/Q3D2PnQP7bkG3e4T1XKzrxcuG9Q5gG/HmTDOgGCr7nZ8S4ssQM 4LyGrS46pVz09XFO03POAicrSiUs6YECnUkDI= To: musl@lists.openwall.com References: <20200624233517.4909-1-daniel.santos@pobox.com> <20200624233517.4909-2-daniel.santos@pobox.com> <20200625004516.GN6430@brightrain.aerifal.cx> From: Daniel Santos Cc: 'Daniel Santos' Message-ID: <8144a7f4-b940-8d62-917c-23517cfd3e5d@pobox.com> Date: Sat, 4 Jul 2020 20:34:26 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.2 MIME-Version: 1.0 In-Reply-To: <20200625004516.GN6430@brightrain.aerifal.cx> Content-Type: text/plain; charset=utf-8 Content-Language: en-US X-Pobox-Relay-ID: F1EE43B8-BE5F-11EA-B467-D1361DBA3BAF-06139138!pb-smtp2.pobox.com Content-Transfer-Encoding: quoted-printable Subject: Re: [musl] [PATCH 1/2] mipsel: Add debug information to __syscall_cp_asm Sorry for my late response.=C2=A0 I didn't realize musl was "reply-to-lis= t only" and my filters didn't flag your reply without the CC.=C2=A0 (Filter= s updated now!) On 6/24/20 7:45 PM, Rich Felker wrote: > On Wed, Jun 24, 2020 at 06:35:16PM -0500, Daniel Santos wrote: >> This is the function called for interruptable / repeatable syscalls li= ke >> nanosleep. Without this patch, attaching a debugger to a program maki= ng >> such a syscall results in the debugger being completely unable to >> perform a backtrace. > On other archs this is handled by the tools/add-cfi* scripts. It might > be worth revisiting whether that's the right choice; for most asm > we're actually trying to get rid of the asm source files and use > inline asm so it's all up to the compiler, but that can't be done for > syscalls because we need to be able to adjust the stack for the > outgoing syscall. (Note: this is probably also an issue for inline, > non-cancellable syscalls and maybe not so easy to fix there.) Yes, there are many things that just cannot be done with inline asm.=C2=A0= I thought we had __attribute__((naked)) for all archs in GCC now, but I guess that doesn't include MIPS (currently looks like ARM, AVR, C-SKY, MCORE, MSP430, NDS32, RISC-V, RL78, RX, SPU, and x86).=C2=A0 I'll take a = look at the scripts, but I'm not strong with awk. > Anyway some comments: > >> Co-Authored-By: Daniele Tamino >> Signed-off-by: Daniel Santos >> --- >> src/thread/mips/syscall_cp.s | 41 +++++++++++++++++++++++++++++++++++= - >> 1 file changed, 40 insertions(+), 1 deletion(-) >> >> diff --git a/src/thread/mips/syscall_cp.s b/src/thread/mips/syscall_cp= .s >> index d2846264..d39bff59 100644 >> --- a/src/thread/mips/syscall_cp.s >> +++ b/src/thread/mips/syscall_cp.s >> @@ -1,4 +1,14 @@ >> +.section .mdebug.abi32 >> +.previous > What does this do? It's a little bit trashy.=C2=A0 I guess that the ".previous" is=C2=A0 use= less since it's followed by a ".text" later.=C2=A0 But the empty .mdebug. section is there to help gdb determine the ABI.=C2=A0 iiuc, newer version= s of binutils generate this automatically. I'm not an expert in this area, or even MIPS yet; I mostly modeled what GCC generated on this arch and then looked up why it did.=C2=A0 According= to GCC, it's there for compatibility with older versions of binutils (and thus, gdb).=C2=A0 https://github.com/gcc-mirror/gcc/blob/98fcd2513add205dcdd134eb29a2505ea9= f81495/gcc/config/mips/mips.c#L9889 .=C2=A0 Seeing how this was added to gcc in 2007, it would probably be sa= fe to omit it, but if you want to keep it then we should remove either the ".previous" or ".text". >> .set noreorder >> +.cfi_sections .debug_frame >> +.abicalls >> +#ifdef __PIC__ >> + .option pic2 >> +#else >> + .option pic0 >> +#endif > I don't think pic0 corresponds to not having -fPIC; it's a weird thing > that's not used or supported. There's no reason to override any of > these (or .abicalls, which is also default and equivalent to pic2 > AIUI), and the change does not seem to be at all related to adding > CFI. Good point -- keep the patch only about debug info. > I'm not sure if .cfi_sections .debug_frame is needed -- is it? > >> +.text >> =20 >> .global __cp_begin >> .hidden __cp_begin >> @@ -9,12 +19,32 @@ >> .global __cp_cancel >> .hidden __cp_cancel >> .type __cp_cancel,@function >> -.hidden __cancel >> +.hidden __cancel /* long __cancel() in src/thread/pthread_cancel.c */ >> .global __syscall_cp_asm >> .hidden __syscall_cp_asm >> .type __syscall_cp_asm,@function >> + >> +/* >> +long __syscall_cp_asm( >> + volatile int *cancel, >> + syscall_arg_t nr, >> + syscall_arg_t u, >> + syscall_arg_t v, >> + syscall_arg_t w, >> + syscall_arg_t x, >> + syscall_arg_t y, >> + syscall_arg_t z) >> +*/ > These changes are also unrelated to CFI. Ah yes.=C2=A0 Just comments when I was analyzing the function. >> + >> + .ent __syscall_cp_asm >> + .frame $sp, 32, $ra >> + .mask 0x00000000, 0 >> + .fmask 0x00000000, 0 >> + .cfi_startproc >> + .cfi_return_column $ra > And I'm not sure about all of these. To my knowledge, .ent, .frame, .mask and .fmask are ignored by gas, but used by MIPSpro for generating debug information -- the exception being that gas expects .ent if you use .end.=C2=A0 If you're only interested in gas, then we should probably omit these.=C2=A0 GCC emits them by default = and finding the documentation for them was a fking pain. http://csweb.cs.wfu.edu/~torgerse/Kokua/Irix_6.5.21_doc_cd/usr/share/Insi= ght/library/SGI_bookshelves/SGI_Developer/books/MProAsLg_PG/sgi_html/ch08= .html ".cfi_startproc" is necessary, but ".cfi_return_column $ra" may be superfluous since it's probably the default after a jump/branch-and-link.=C2=A0 I'll test without it and see. >> __syscall_cp_asm: >> subu $sp, $sp, 32 >> + .cfi_adjust_cfa_offset 32 >> __cp_begin: >> lw $4, 0($4) >> bne $4, $0, __cp_cancel >> @@ -35,14 +65,17 @@ __cp_begin: >> __cp_end: >> beq $7, $0, 1f >> addu $sp, $sp, 32 >> + .cfi_adjust_cfa_offset -32 >> subu $2, $0, $2 >> 1: jr $ra >> nop >> =20 >> __cp_cancel: >> move $2, $ra >> + .cfi_register $ra, $2 >> bal 1f >> addu $sp, $sp, 32 >> + .cfi_adjust_cfa_offset -32 >> .gpword . >> .gpword __cancel >> 1: lw $3, ($ra) >> @@ -51,3 +84,9 @@ __cp_cancel: >> addu $25, $25, $3 >> jr $25 >> move $ra, $2 > I think this part looks ok, either to be generated like on the other > archs or included. I'm not certain the ".cfi_register $ra, $2" can be safely added via a script and it could probably even be moved to after the bal delay slot, along with the ".cfi_adjust_cfa_offset -32".=C2=A0 In fact, I seem to rec= all that "stepi" in gdb used to step "over" branch instructions and then branch after using "stepi" on the delay slot -- I've noticed recently that it now executes both at once. >> + .cfi_restore $ra >> +#ifdef __ELF__ >> + .size __syscall_cp_asm,.-__syscall_cp_asm >> +#endif > This is not needed. musl is always ELF. Fantastic!=C2=A0 So if we also don't need __PIC__ then we can omit the re= name to *.S. > Rich I would also like to do the same thing for clone and dlsym.=C2=A0 I'll fi= ddle with the awk scripts and see what I can get them to do when I get some more time in the next week. Thanks, Daniel