From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.88; helo=mga01.intel.com; envelope-from=steven.shi@intel.com; receiver=edk2-devel@lists.01.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D8FE7209603ED for ; Mon, 2 Jul 2018 02:22:16 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 02 Jul 2018 02:22:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,298,1526367600"; d="scan'208";a="53797349" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga008.jf.intel.com with ESMTP; 02 Jul 2018 02:22:15 -0700 Received: from fmsmsx151.amr.corp.intel.com (10.18.125.4) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 2 Jul 2018 02:22:15 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX151.amr.corp.intel.com (10.18.125.4) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 2 Jul 2018 02:22:15 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.223]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.82]) with mapi id 14.03.0319.002; Mon, 2 Jul 2018 17:22:12 +0800 From: "Shi, Steven" To: Zenith432 , "edk2-devel@lists.01.org" CC: "Gao, Liming" Thread-Topic: [edk2] [PATCH v2] BaseTools/GenFw: Add X64 GOTPCREL Support to GenFw Thread-Index: AQHT/l4Qu0YaMkc62kub4zTpi/8eg6R7vLqQ Date: Mon, 2 Jul 2018 09:22:12 +0000 Message-ID: <06C8AB66E78EE34A949939824ABE2B313B791561@shsmsx102.ccr.corp.intel.com> References: <1917777633.1686290.1528375781778.ref@mail.yahoo.com> <1917777633.1686290.1528375781778@mail.yahoo.com> In-Reply-To: <1917777633.1686290.1528375781778@mail.yahoo.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZDU5NGI3YzAtZjQxMS00NDBkLWI5NzUtZGZhNGFkYjAwMDYzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiY1NYSW9EQmRjZlZ4ZjJGdFwvUXJpNXhKOUVTdnZ5c0lWOTlITDF2amtyM00zMTlwU2tydVNQenlJcGo2eUJEM0QifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v2] BaseTools/GenFw: Add X64 GOTPCREL Support to GenFw X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 02 Jul 2018 09:22:18 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Zenith, I like your patch. Sorry for the late response. I see your patch has no im= pact to current supported relocation types, and I also hope we can add more= relocation types in our GenFw tool.=20 Below are some code style issues in your patch: =20 > +STATIC > +VOID > +FindElfGOTSectionFromGOTEntryElfRva ( > + Elf64_Addr GOTEntryElfRva > + ) > +{ > + UINT32 i; > + if (mGOTShdr !=3D NULL) { > + if (GOTEntryElfRva >=3D mGOTShdr->sh_addr && > + GOTEntryElfRva < mGOTShdr->sh_addr + mGOTShdr->sh_size) > + return; We usually use the {} to brace the if branch statements, even it has only = one statement, like this: if (GOTEntryElfRva >=3D mGOTShdr->sh_addr && GOTEntryElfRva < mGOTShdr->sh_addr + mGOTShdr->sh_size){ return; } > +STATIC > +BOOLEAN > +AccumulateCoffGOTEntries ( > + UINT32 GOTCoffEntry > + ) > +{ > + UINT32 i; > + if (mGOTCoffEntries !=3D NULL) { > + for (i =3D 0; i < mGOTNumCoffEntries; i++) > + if (mGOTCoffEntries[i] =3D=3D GOTCoffEntry) > + return FALSE; Same issue as above, we usually always use {} to include if true branch sta= tement like this: if (mGOTCoffEntries[i] =3D=3D GOTCoffEntry){ return FALSE; } > +STATIC > +int > +__comparator ( > + const void* lhs, > + const void* rhs > + ) > +{ > + if (*(const UINT32*)lhs < *(const UINT32*)rhs) > + return -1; > + return *(const UINT32*)lhs > *(const UINT32*)rhs; > +} We usually don't use '__' as prefix for a function, could you enhance the c= omparator() name and add {} to include if branch statement? > +STATIC > +VOID > +EmitGOTRelocations ( > + VOID > + ) > +{ > + UINT32 i; > + if (mGOTCoffEntries =3D=3D NULL) > + return; Please add {} to include if branch statement > + if (mEhdr->e_machine =3D=3D EM_X86_64 && RelShdr->sh_info =3D=3D > mGOTShindex) { > + // > + // Tack relocations for GOT entries after other relocations fo= r > + // the section the GOT is in, as it's usually found at the e= nd > + // of the section. > + // > + EmitGOTRelocations(); > + } > } > } > } >=20 > + if (mEhdr->e_machine =3D=3D EM_X86_64) { > + // > + // Just in case GOT is in a section with no other relocations > + // > + EmitGOTRelocations(); > + } Could we combine the two EmitGOTRelocations() invokes and only keep the sec= ond one? Thanks Steven