Message ID | CAKCAbMiZ6S4uW9PB8f=b-W4azN-k20R9aFnJedvmt=i3cfiDVA@mail.gmail.com |
---|---|
State | Committed |
Headers |
Received: (qmail 29114 invoked by alias); 22 May 2017 20:11:44 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 29088 invoked by uid 89); 22 May 2017 20:11:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_SORBS_SPAM, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 spammy=paying, CVS, xxx X-Spam-User: qpsmtpd, 2 recipients X-HELO: mailbackend.panix.com X-Gm-Message-State: AODbwcA3/LBCow0vgt3a6PoZ3M/u9Xj72Bve7v0mLnAOseJ6QH1Cq/NQ AxZlg6olQUHmS/xDjRDMWDnKB+y5SA== X-Received: by 10.36.104.72 with SMTP id v69mr41060971itb.27.1495483903064; Mon, 22 May 2017 13:11:43 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <1495482453-6499-1-git-send-email-siddhesh@sourceware.org> References: <1495482453-6499-1-git-send-email-siddhesh@sourceware.org> From: Zack Weinberg <zackw@panix.com> Date: Mon, 22 May 2017 16:11:42 -0400 X-Gmail-Original-Message-ID: <CAKCAbMiZ6S4uW9PB8f=b-W4azN-k20R9aFnJedvmt=i3cfiDVA@mail.gmail.com> Message-ID: <CAKCAbMiZ6S4uW9PB8f=b-W4azN-k20R9aFnJedvmt=i3cfiDVA@mail.gmail.com> Subject: Re: [COMMITTED] Remove useless comment from sysdeps/sparc/sparc32/dl-machine.h To: Siddhesh Poyarekar <siddhesh@sourceware.org>, jakub@redhat.com Cc: GNU C Library <libc-alpha@sourceware.org> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable |
Commit Message
Zack Weinberg
May 22, 2017, 8:11 p.m. UTC
On Mon, May 22, 2017 at 3:47 PM, Siddhesh Poyarekar <siddhesh@sourceware.org> wrote: > - /* XXX The following is wrong! Dave Miller rejected to implement it > - correctly. If this causes problems shoot *him*! */ You got me curious, so I did some archaeology. This comment was introduced in 1999, and unfortunately Drepper seems to have been in the habit of copying changes out of what I assume was a Cygnus Inc internal CVS repository in large batches, so the commit message is unhelpful, but the original patch to this file (https://sourceware.org/git/?p=glibc.git;a=blobdiff;f=sysdeps/sparc/sparc32/dl-machine.h;h=8750791fe0cf20f55250ad4c2147ebb992cadb93;hp=e6debe05ac511232cad7856be66298359f32e7b4;hb=63ae7b6309964400c6289d9030c7d336a33b304f;hpb=b4cbd37109d882aa052e7973684774eaa887071b) was and, if I'm reading the ChangeLog correctly, the true author was Jakub Jelinek, who I've cc:ed just on the offchance he remembers what was going on. Note that this changes the sense of the test from "the hwcap is available AND the flag HWCAP_SPARC_V9 is set" to "the hwcap is NOT available OR the flag HWCAP_SPARC_V9 is set". I do not know for sure, but what this smells like to me is there may have been a bunch of SPARCv9 binaries that were generated before hwcaps were introduced, so they stopped working when ld.so started paying attention to hwcaps, so Miller wanted to relax the rules and Jelinek didn't like that. What's in the file *now* appears to have reverted to the old logic (and that happened in 2003), so if my nose is correct, then the comment is indeed completely obsolete. zw
Comments
From: Zack Weinberg <zackw@panix.com> Date: Mon, 22 May 2017 16:11:42 -0400 > On Mon, May 22, 2017 at 3:47 PM, Siddhesh Poyarekar > <siddhesh@sourceware.org> wrote: >> - /* XXX The following is wrong! Dave Miller rejected to implement it >> - correctly. If this causes problems shoot *him*! */ > > You got me curious, so I did some archaeology. This comment was > introduced in 1999, and unfortunately Drepper seems to have been in > the habit of copying changes out of what I assume was a Cygnus Inc > internal CVS repository in large batches, so the commit message is > unhelpful, but the original patch to this file > (https://sourceware.org/git/?p=glibc.git;a=blobdiff;f=sysdeps/sparc/sparc32/dl-machine.h;h=8750791fe0cf20f55250ad4c2147ebb992cadb93;hp=e6debe05ac511232cad7856be66298359f32e7b4;hb=63ae7b6309964400c6289d9030c7d336a33b304f;hpb=b4cbd37109d882aa052e7973684774eaa887071b) > was > > --- a/sysdeps/sparc/sparc32/dl-machine.h > +++ b/sysdeps/sparc/sparc32/dl-machine.h > @@ -56,7 +56,9 @@ elf_machine_matches_host (Elf32_Half e_machine) > weak_extern (_dl_hwcap_mask); > > hwcap = WEAKADDR(_dl_hwcap); > - return hwcap && (*hwcap & _dl_hwcap_mask & HWCAP_SPARC_V9); > + /* XXX The following is wrong! Dave Miller rejected to implement it > + correctly. If this causes problems shoot *him*! */ > + return hwcap == NULL || (*hwcap & _dl_hwcap_mask & HWCAP_SPARC_V9); > } > else > return 0; > > and, if I'm reading the ChangeLog correctly, the true author was Jakub > Jelinek, who I've cc:ed just on the offchance he remembers what was > going on. > > Note that this changes the sense of the test from "the hwcap is > available AND the flag HWCAP_SPARC_V9 is set" to "the hwcap is NOT > available OR the flag HWCAP_SPARC_V9 is set". I do not know for sure, > but what this smells like to me is there may have been a bunch of > SPARCv9 binaries that were generated before hwcaps were introduced, so > they stopped working when ld.so started paying attention to hwcaps, so > Miller wanted to relax the rules and Jelinek didn't like that. The context is that Drepper didn't like the change, Jakub and I were all for it. > What's in the file *now* appears to have reverted to the old logic > (and that happened in 2003), so if my nose is correct, then the > comment is indeed completely obsolete. Even more reason to remove the stale comment.
On Mon, 22 May 2017, Zack Weinberg wrote: > You got me curious, so I did some archaeology. This comment was > introduced in 1999, and unfortunately Drepper seems to have been in > the habit of copying changes out of what I assume was a Cygnus Inc > internal CVS repository in large batches, so the commit message is > unhelpful, but the original patch to this file My understanding is: commits were done (with Emacs VC?) with separate per-file log messages. We tried to combine successive commits like that, finishing with one committing to ChangeLog, for the conversion from CVS to git, but while the heuristic of taking a sequence of consecutive commits ending with one committing to ChangeLog was effective for the more recent commits at the time of the conversion, the older commits did not involve frequent commits of ChangeLog files, so more changes got combined than desirable. See git://repo.or.cz/glibc/history for a straight conversion of the CVS history without any such heuristics used to combine commits.
--- a/sysdeps/sparc/sparc32/dl-machine.h +++ b/sysdeps/sparc/sparc32/dl-machine.h @@ -56,7 +56,9 @@ elf_machine_matches_host (Elf32_Half e_machine) weak_extern (_dl_hwcap_mask); hwcap = WEAKADDR(_dl_hwcap); - return hwcap && (*hwcap & _dl_hwcap_mask & HWCAP_SPARC_V9); + /* XXX The following is wrong! Dave Miller rejected to implement it + correctly. If this causes problems shoot *him*! */ + return hwcap == NULL || (*hwcap & _dl_hwcap_mask & HWCAP_SPARC_V9); } else return 0;