[COMMITTED] Remove useless comment from sysdeps/sparc/sparc32/dl-machine.h

Message ID CAKCAbMiZ6S4uW9PB8f=b-W4azN-k20R9aFnJedvmt=i3cfiDVA@mail.gmail.com
State Committed
Headers

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

David Miller May 22, 2017, 8:20 p.m. UTC | #1
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.
  
Joseph Myers May 22, 2017, 8:22 p.m. UTC | #2
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.
  

Patch

--- 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;