From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 60988 invoked by alias); 17 Dec 2019 23:59:26 -0000 Mailing-List: contact binutils-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: binutils-owner@sourceware.org Received: (qmail 60980 invoked by uid 89); 17 Dec 2019 23:59:26 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy= X-HELO: mail-vs1-f49.google.com Received: from mail-vs1-f49.google.com (HELO mail-vs1-f49.google.com) (209.85.217.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 17 Dec 2019 23:59:24 +0000 Received: by mail-vs1-f49.google.com with SMTP id t12so219031vso.13 for ; Tue, 17 Dec 2019 15:59:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sifive.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=HKa4/sEyuixvzmjsyZ55u7/wIoqHViyStk+ev8lwsMI=; b=iQ7GtWbVBvjni9q8akWcVJVMME27Zcst3epBA+ZiLGEd2ZhCsl7SlPtV79TkHgkNvG ONR6WtEK8ljpsGVTN4pllrCLAHgifjOsrnz6r1iXe6yC6nN+03xjfJ+wX5jriRiomj+6 W1G0i/AcRBBC8nOdv1hU0KiOiP/xDGl4VU1M+sgQMpsJ7ZH0kAegf61bXSg8SSoVh8h8 oaWJyOD5SnFoxNywlqiR+pG4Hh5voPoKFdhTNr7o5pbLjLOcJd3B/is6s8uNkTFh32ZL wKyVaZ7ntWf1TWjOh5tjWsLCeewADKTyyxrlZCUEC2EOc3zWiQXFEB2Po7iJA2oHnz9S lu9g== MIME-Version: 1.0 References: In-Reply-To: From: Jim Wilson Date: Tue, 17 Dec 2019 23:59:00 -0000 Message-ID: Subject: Re: [PATCH] [RISCV] Support subtraction of .uleb128. To: Kuan-Lin Chen Cc: Palmer Dabbelt , "binutils@sourceware.org Development" Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-12/txt/msg00279.txt.bz2 I tried the patch, and it works for me. It even fixes one gcc testsuite failure oddly enough, gcc.dg/debug/dwarf2/inline5.c, I didn't look at the details of why. I see that there is no actual relaxation of the uleb128. We just pick the size at assembly time via a variant frag, and then assuming that the value can never increase during relaxation, we just need to add enough leading zeros at link time to ensure that the size of the uleb128 doesn't change if the value gets smaller. It is a little worrying that we can only support uleb128 and not sleb128 here, since this trick won't work for sleb128, but I don't think that is a problem. Just handling uleb128 is good enough to allow gcc to use uleb128/sleb128, as we only use subtraction on addresses, and they are always treated as unsigned. I see that object files get a bit larger due to the extra relocations, but executables end up smaller because the gcc_except_table is now smaller. A simple rv32 C++ hello world program linked against a static libstdc++ reduces in size by about 2.4% with this patch. The assumption that the uleb128 value always decreases could be wrong if we are subtracting a text section address from a data section address, because the size of alignment padding between the two can increase if the text section size decreases. This code could fail in this case. I don't believe that gcc ever does this, but an end user could conceivably write assembly code that does this which could be a problem. We could handle this in the assembler by forcing the uleb128 size to the pointer size if we are subtracting two pointers that aren't in the same section. I think that being in the same section is enough, and that we don't have to require them to be in the same frag because of how we handle alignment when relaxing. That would make the patch safer, and shouldn't change the size reduction we get with it. This may require some inconvenient code duplication though, since you are currently using the generic s_leb128 support which doesn't do this. Unless maybe we can add a target dependent hook there to avoid copying it. There is the issue I mentioned before that I'd like to see the psABI updated before we modify binutils to use the new relocs. Letting gcc emit code that llvm can't link will cause problems. Once this patch goes in, gcc will automatically start emitting the new relocs because a configure test that currently fails will start working. I added some info to the psABI issue to help, but I think we really need some feedback from the LLVM folks before we commit this. Otherwise, I don't see anything wrong with the patch other than a few style issues. These are all minor problems. + endp = p + len -1; + memset (p, 0x80, len); + *(endp) = 0; Seem like the memset should be len - 1 but it seems harmless because we just set the *endp value twice. + /* The length of unsigned-leb128 is variant, just assume the + size is one byte here. */ I'd use "variable" instead of "variant", but that is just a style issue. This happens in two places. + Becuase relaxation may change the value of the subtraction, we + must resolve them in link-time. */ Becuase -> Because in -> at + /* Only unsigned leb128 can be handle. */ handle -> handled + /* Insert relocations to resolve the subtraction in link-time. */ in -> at Jim