From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail8.parnet.fi (mail8.parnet.fi [77.234.108.134]) by sourceware.org (Postfix) with ESMTPS id 047EB3858006 for ; Mon, 10 Oct 2022 10:27:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 047EB3858006 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=martin.st Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=martin.st Received: from mail9.parnet.fi (mail9.parnet.fi [77.234.108.21]) by mail8.parnet.fi with ESMTP id 29AARX4l016817-29AARX4m016817; Mon, 10 Oct 2022 13:27:33 +0300 Received: from foo.martin.st (host-97-187.parnet.fi [77.234.97.187]) by mail9.parnet.fi (Postfix) with ESMTPS id 27D52A1437; Mon, 10 Oct 2022 13:27:32 +0300 (EEST) Date: Mon, 10 Oct 2022 13:27:31 +0300 (EEST) From: =?ISO-8859-15?Q?Martin_Storsj=F6?= To: Mark Harmstone cc: binutils@sourceware.org Subject: Re: [PATCH 1/2] ld: Add --pdb option In-Reply-To: <2e5696c4-57db-6cd1-603b-e106754b70f5@harmstone.com> Message-ID: <50a83eb4-feaa-9cf9-bb40-42bff718796f@martin.st> References: <20221003014313.28766-1-mark@harmstone.com> <26dfc8b7-e89d-9212-da69-b05044d2d8a9@martin.st> <7e5d88ff-9aa9-dbfd-aa80-1793c2c48fde@martin.st> <2b3caba0-de53-6c2c-1038-5581deb2b8e@martin.st> <2e5696c4-57db-6cd1-603b-e106754b70f5@harmstone.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323329-899881569-1665397653=:1659" X-FE-Attachment-Name: 0001-squash-ld-pdb-Switch-the-pdb-option-to-required_argu.patch X-FE-Policy-ID: 3:14:2:SYSTEM X-Spam-Status: No, score=-7.9 required=5.0 tests=BAYES_00,BODY_8BITS,GIT_PATCH_0,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,KAM_LOTSOFHASH,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-899881569-1665397653=:1659 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8BIT On Mon, 10 Oct 2022, Mark Harmstone wrote: > On 7/10/22 13:16, Martin Storsjö wrote: >> On Mon, 3 Oct 2022, Martin Storsjö wrote: >> >>> On Mon, 3 Oct 2022, Mark Harmstone wrote: >>> >>>> Hi Martin, >>>> >>>>> As I assume you're aware, lld's mingw port also supports PDB generation >>>>> - and the description of this option also sounds like it's chosen to >>>>> match lld's option for outputting PDB files - that's good! >>>> >>>> Yes, that's right. One notable difference is that the parameter here is >>>> optional, unlike with lld, making it a lot easier to fit this into e.g. >>>> CMake toolchain files or LDFLAGS. >>> >>> LLD also has got that behaviour, since >>> https://github.com/llvm/llvm-project/commit/2c52ddf31f5421c5373923535b958b84c79772e3 >>> in 2019. That's in particular why I wanted to make sure that this case >>> works the same in binutils too. >>> >>>> It looks like the equals sign is mandatory when providing optional >>>> parameters, otherwise it interprets the filename as another parameter. >>> >>> Yep, that's the case in LLD too. >>> >>> Unfortunately I didn't think of this behaviour initially when I first >>> added this option - otherwise we could have had e.g. --pdb as a boolean >>> option to just output to the default name, and e.g. --output-pdb= if >>> you wanted to specify the name. But oh well, "-pdb=" works, and I guess it >>> isn't the worst thing in the world. >>> >>>> But it does mean that the form "-pdb=out.pdb" will work on both ld and >>>> lld, which I think is the most important thing. >>> >>> TBH, I consider the "-pdb=" case equally important too - that's what most >>> people would use in the end. >> >> FWIW, I'm actually a bit concerned about the interop between binutils and >> lld here. I don't want interop between binutils and lld to work only for >> some subset of the used parameter forms, I'd like it to work for all >> commonly used forms. >> >> >> First off, the (slightly awkward) syntax that lld uses for an optional >> empty output name, "-pdb=" really should be handled by binutils too - >> handling that doesn't conflict with anything else and should be simple to >> support. >> >> This is the format of the option that I've been recommending people to use, >> and this has been in use in third party projects for years already - e.g. >> this: >> https://code.videolan.org/videolan/vlc/-/blob/master/configure.ac#L429 >> >> This should be trivial to support in your patch: >> >> diff --git a/ld/emultempl/pep.em b/ld/emultempl/pep.em >> index 11216830dd3..538fdf5054b 100644 >> --- a/ld/emultempl/pep.em >> +++ b/ld/emultempl/pep.em >> @@ -926,7 +926,7 @@ gld${EMULATION_NAME}_handle_option (int optc) >>        if (emit_build_id == NULL) >>         emit_build_id = xstrdup (DEFAULT_BUILD_ID_STYLE); >>        pdb = 1; >> -      if (optarg) >> +      if (optarg && optarg[0]) >>         pdb_name = xstrdup (optarg); >>        break; >>      } >> >> (And the same for pe.em.) >> >> >> Secondly, for explicitly naming an output file, I've documented to end >> users that they can use either -Wl,-pdb= or -Wl,-pdb, - >> see >> https://github.com/mstorsjo/llvm-mingw/blob/master/README.md?plain=1#L175. >> >> In the original implementation in the mingw frontend in lld in 2018, the >> "-pdb " format was the only format for the option: >> https://github.com/llvm/llvm-project/commit/b7d50115ba4900da6db7afb6460ad42ff19ba6a2 >> >> Only one year later with the implicit output name, the "-pdb=" and >> "-pdb=" form was added: >> https://github.com/llvm/llvm-project/commit/2c52ddf31f5421c5373923535b958b84c79772e3 >> >> In one of my test scripts, I use the initial form of the option, >> -Wl,-pdb,: >> https://github.com/mstorsjo/llvm-mingw/blob/master/run-tests.sh#L234 >> >> It seems like Wine has picked up on the -Wl,-pdb, form: >> https://gitlab.winehq.org/wine/wine/-/blob/wine-7.18/tools/winegcc/winegcc.c#L467 >> >> Also here are a couple of other cases I found that all seem to use that >> form: >> https://youtrack.jetbrains.com/issue/KT-47175/How-to-generate-kotlin-native-debug-info-filesPDB-on-windows-platform >> https://git.kernel.dk/?p=fio.git;a=commitdiff;h=76bc30ca118fda404f19c17d97bafdba9779c4c2 >> >> So with all these users, I'd be kinda hesitant to change lld's >> interpretation of this option form, and to have binutils ld in parallel >> interpreting that form differently. What do you think? >> >> >> // Martin > Hi Martin, > > Fair enough - I'm not overly wedded to this, and will change it if, as you > say, it'll cause issues elsewhere. Ok, great, thanks! However this patchset also lost the ability to get an automatically chosen output file name, which currently is used via the slightly awkward syntax "--pdb=" without an empty parameter. I see you refactored a bit of code in this revision of the patch, which lost that ability. With the patch I'm attaching, applied on top of v1 of your patch, I think it behaves as a reasonable compromise; getopt's required_argument does allow the --pdb= form too (which I think is the one we still should recommend going forward), and passing "--pdb=" allows implying the automatic naming behaviour. // Martin --8323329-899881569-1665397653=:1659 Content-Type: text/x-diff; name=0001-squash-ld-pdb-Switch-the-pdb-option-to-required_argu.patch Content-Transfer-Encoding: BASE64 Content-ID: Content-Description: Content-Disposition: attachment; filename=0001-squash-ld-pdb-Switch-the-pdb-option-to-required_argu.patch RnJvbSBkOGM2NGMzZmI4ZGFjYmJmN2M4ZGNiYzJlMzk0YTJmOWNmYzhiZDIw IE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQ0KRnJvbTogPT9VVEYtOD9xP01h cnRpbj0yMFN0b3Jzaj1DMz1CNj89IDxtYXJ0aW5AbWFydGluLnN0Pg0KRGF0 ZTogTW9uLCAxMCBPY3QgMjAyMiAxMzoyMzoyNiArMDMwMA0KU3ViamVjdDog W1BBVENIXSBzcXVhc2g6IGxkOiBwZGI6IFN3aXRjaCB0aGUgLS1wZGIgb3B0 aW9uIHRvIHJlcXVpcmVkX2FyZ3VtZW50DQoNCkFsbG93IHBhc3NpbmcgYW4g ZW1wdHkgcGFyYW1ldGVyIHRvIGl0LCB0byBzaWduYWwgYW4gYXV0b21hdGlj YWxseQ0KY2hvc2VuIGZpbGUgbmFtZS4NCi0tLQ0KIGxkL2VtdWx0ZW1wbC9w ZS5lbSAgfCA2ICsrKy0tLQ0KIGxkL2VtdWx0ZW1wbC9wZXAuZW0gfCA2ICsr Ky0tLQ0KIDIgZmlsZXMgY2hhbmdlZCwgNiBpbnNlcnRpb25zKCspLCA2IGRl bGV0aW9ucygtKQ0KDQpkaWZmIC0tZ2l0IGEvbGQvZW11bHRlbXBsL3BlLmVt IGIvbGQvZW11bHRlbXBsL3BlLmVtDQppbmRleCBhYmJlZTYyZmQ2Ni4uMmYw NTMwMzgyNmQgMTAwNjQ0DQotLS0gYS9sZC9lbXVsdGVtcGwvcGUuZW0NCisr KyBiL2xkL2VtdWx0ZW1wbC9wZS5lbQ0KQEAgLTM4OCw3ICszODgsNyBAQCBn bGQke0VNVUxBVElPTl9OQU1FfV9hZGRfb3B0aW9ucw0KICAgICB7InRzYXdh cmUiLCBub19hcmd1bWVudCwgTlVMTCwgT1BUSU9OX1RFUk1JTkFMX1NFUlZF Ul9BV0FSRX0sDQogICAgIHsiZGlzYWJsZS10c2F3YXJlIiwgbm9fYXJndW1l bnQsIE5VTEwsIE9QVElPTl9ESVNBQkxFX1RFUk1JTkFMX1NFUlZFUl9BV0FS RX0sDQogICAgIHsiYnVpbGQtaWQiLCBvcHRpb25hbF9hcmd1bWVudCwgTlVM TCwgT1BUSU9OX0JVSUxEX0lEfSwNCi0gICAgeyJwZGIiLCBvcHRpb25hbF9h cmd1bWVudCwgTlVMTCwgT1BUSU9OX1BEQn0sDQorICAgIHsicGRiIiwgcmVx dWlyZWRfYXJndW1lbnQsIE5VTEwsIE9QVElPTl9QREJ9LA0KICAgICB7ImVu YWJsZS1yZWxvYy1zZWN0aW9uIiwgbm9fYXJndW1lbnQsIE5VTEwsIE9QVElP Tl9FTkFCTEVfUkVMT0NfU0VDVElPTn0sDQogICAgIHsiZGlzYWJsZS1yZWxv Yy1zZWN0aW9uIiwgbm9fYXJndW1lbnQsIE5VTEwsIE9QVElPTl9ESVNBQkxF X1JFTE9DX1NFQ1RJT059LA0KICAgICB7TlVMTCwgbm9fYXJndW1lbnQsIE5V TEwsIDB9DQpAQCAtNTM4LDcgKzUzOCw3IEBAIGdsZCR7RU1VTEFUSU9OX05B TUV9X2xpc3Rfb3B0aW9ucyAoRklMRSAqZmlsZSkNCiAgIGZwcmludGYgKGZp bGUsIF8oIiAgLS1bZGlzYWJsZS1dd2RtZHJpdmVyICAgICAgICAgICAgICBE cml2ZXIgdXNlcyB0aGUgV0RNIG1vZGVsXG4iKSk7DQogICBmcHJpbnRmIChm aWxlLCBfKCIgIC0tW2Rpc2FibGUtXXRzYXdhcmUgICAgICAgICAgICAgICAg SW1hZ2UgaXMgVGVybWluYWwgU2VydmVyIGF3YXJlXG4iKSk7DQogICBmcHJp bnRmIChmaWxlLCBfKCIgIC0tYnVpbGQtaWRbPVNUWUxFXSAgICAgICAgICAg ICAgICAgR2VuZXJhdGUgYnVpbGQgSURcbiIpKTsNCi0gIGZwcmludGYgKGZp bGUsIF8oIiAgLS1wZGJbPUZJTEVOQU1FXSAgICAgICAgICAgICAgICAgICBH ZW5lcmF0ZSBQREIgZmlsZVxuIikpOw0KKyAgZnByaW50ZiAoZmlsZSwgXygi ICAtLXBkYj1bRklMRU5BTUVdICAgICAgICAgICAgICAgICAgIEdlbmVyYXRl IFBEQiBmaWxlXG4iKSk7DQogfQ0KIA0KIC8qIEEgY2FzZSBpbnNlbnNpdGl2 ZSBjb21wYXJpc29uLCByZWdhcmRsZXNzIG9mIHRoZSBob3N0IHBsYXRmb3Jt LCB1c2VkIGZvcg0KQEAgLTk4Myw3ICs5ODMsNyBAQCBnbGQke0VNVUxBVElP Tl9OQU1FfV9oYW5kbGVfb3B0aW9uIChpbnQgb3B0YykNCiAgICAgICBpZiAo ZW1pdF9idWlsZF9pZCA9PSBOVUxMKQ0KIAllbWl0X2J1aWxkX2lkID0geHN0 cmR1cCAoREVGQVVMVF9CVUlMRF9JRF9TVFlMRSk7DQogICAgICAgcGRiID0g MTsNCi0gICAgICBpZiAob3B0YXJnKQ0KKyAgICAgIGlmIChvcHRhcmcgJiYg b3B0YXJnWzBdKQ0KIAlwZGJfbmFtZSA9IHhzdHJkdXAgKG9wdGFyZyk7DQog ICAgICAgYnJlYWs7DQogICAgIH0NCmRpZmYgLS1naXQgYS9sZC9lbXVsdGVt cGwvcGVwLmVtIGIvbGQvZW11bHRlbXBsL3BlcC5lbQ0KaW5kZXggMTEyMTY4 MzBkZDMuLjI2NmVhOTY5MmMyIDEwMDY0NA0KLS0tIGEvbGQvZW11bHRlbXBs L3BlcC5lbQ0KKysrIGIvbGQvZW11bHRlbXBsL3BlcC5lbQ0KQEAgLTM2NSw3 ICszNjUsNyBAQCBnbGQke0VNVUxBVElPTl9OQU1FfV9hZGRfb3B0aW9ucw0K ICAgICB7Imluc2VydC10aW1lc3RhbXAiLCBub19hcmd1bWVudCwgTlVMTCwg T1BUSU9OX0lOU0VSVF9USU1FU1RBTVB9LA0KICAgICB7Im5vLWluc2VydC10 aW1lc3RhbXAiLCBub19hcmd1bWVudCwgTlVMTCwgT1BUSU9OX05PX0lOU0VS VF9USU1FU1RBTVB9LA0KICAgICB7ImJ1aWxkLWlkIiwgb3B0aW9uYWxfYXJn dW1lbnQsIE5VTEwsIE9QVElPTl9CVUlMRF9JRH0sDQotICAgIHsicGRiIiwg b3B0aW9uYWxfYXJndW1lbnQsIE5VTEwsIE9QVElPTl9QREJ9LA0KKyAgICB7 InBkYiIsIHJlcXVpcmVkX2FyZ3VtZW50LCBOVUxMLCBPUFRJT05fUERCfSwN CiAgICAgeyJlbmFibGUtcmVsb2Mtc2VjdGlvbiIsIG5vX2FyZ3VtZW50LCBO VUxMLCBPUFRJT05fRU5BQkxFX1JFTE9DX1NFQ1RJT059LA0KICAgICB7ImRp c2FibGUtcmVsb2Mtc2VjdGlvbiIsIG5vX2FyZ3VtZW50LCBOVUxMLCBPUFRJ T05fRElTQUJMRV9SRUxPQ19TRUNUSU9OfSwNCiAgICAgeyJkaXNhYmxlLWhp Z2gtZW50cm9weS12YSIsIG5vX2FyZ3VtZW50LCBOVUxMLCBPUFRJT05fRElT QUJMRV9ISUdIX0VOVFJPUFlfVkF9LA0KQEAgLTUxMyw3ICs1MTMsNyBAQCBn bGQke0VNVUxBVElPTl9OQU1FfV9saXN0X29wdGlvbnMgKEZJTEUgKmZpbGUp DQogICBmcHJpbnRmIChmaWxlLCBfKCIgIC0tW2Rpc2FibGUtXXdkbWRyaXZl ciAgICAgICAgICAgICAgRHJpdmVyIHVzZXMgdGhlIFdETSBtb2RlbFxuIikp Ow0KICAgZnByaW50ZiAoZmlsZSwgXygiICAtLVtkaXNhYmxlLV10c2F3YXJl ICAgICAgICAgICAgICAgIEltYWdlIGlzIFRlcm1pbmFsIFNlcnZlciBhd2Fy ZVxuIikpOw0KICAgZnByaW50ZiAoZmlsZSwgXygiICAtLWJ1aWxkLWlkWz1T VFlMRV0gICAgICAgICAgICAgICAgIEdlbmVyYXRlIGJ1aWxkIElEXG4iKSk7 DQotICBmcHJpbnRmIChmaWxlLCBfKCIgIC0tcGRiWz1GSUxFTkFNRV0gICAg ICAgICAgICAgICAgICAgR2VuZXJhdGUgUERCIGZpbGVcbiIpKTsNCisgIGZw cmludGYgKGZpbGUsIF8oIiAgLS1wZGI9W0ZJTEVOQU1FXSAgICAgICAgICAg ICAgICAgICBHZW5lcmF0ZSBQREIgZmlsZVxuIikpOw0KICNlbmRpZg0KIH0N CiANCkBAIC05MjYsNyArOTI2LDcgQEAgZ2xkJHtFTVVMQVRJT05fTkFNRX1f aGFuZGxlX29wdGlvbiAoaW50IG9wdGMpDQogICAgICAgaWYgKGVtaXRfYnVp bGRfaWQgPT0gTlVMTCkNCiAJZW1pdF9idWlsZF9pZCA9IHhzdHJkdXAgKERF RkFVTFRfQlVJTERfSURfU1RZTEUpOw0KICAgICAgIHBkYiA9IDE7DQotICAg ICAgaWYgKG9wdGFyZykNCisgICAgICBpZiAob3B0YXJnICYmIG9wdGFyZ1sw XSkNCiAJcGRiX25hbWUgPSB4c3RyZHVwIChvcHRhcmcpOw0KICAgICAgIGJy ZWFrOw0KICAgICB9DQotLSANCjIuMjUuMQ0KDQo= --8323329-899881569-1665397653=:1659--