diff mbox series

[FYI,ppc] adjust configure test for large toc support

Message ID or348rxd6y.fsf@lxoliva.fsfla.org
State New
Headers
Series [FYI,ppc] adjust configure test for large toc support |

Commit Message

Alexandre Oliva Sept. 13, 2025, 3:07 a.m. UTC 
  The use of the TLS register in a TOC/GOT address computation was
probably a cut&pasto or a thinko.  It causes a linker warning and,
because the TLS access in the test is incomplete, may cause
significant confusion.  Adjust to use the TOC/GOT register as base.

Tested manually with cross binutils for ppc64-linux-gnu and
ppc64-vxworks7r2.  Unless there's any objection in the next few days, I
plan to put this in along with other changes.

for  gcc/ChangeLog

	* configure.ac: Adjust base register in linker test for large
	toc support.
	* configure: Rebuilt.
---
 gcc/configure    |    2 +-
 gcc/configure.ac |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Segher Boessenkool Sept. 13, 2025, 8:04 a.m. UTC | #1 
Hi!

On Sat, Sep 13, 2025 at 12:07:33AM -0300, Alexandre Oliva wrote:
> The use of the TLS register in a TOC/GOT address computation was
> probably a cut&pasto or a thinko.  It causes a linker warning and,
> because the TLS access in the test is incomplete, may cause
> significant confusion.  Adjust to use the TOC/GOT register as base.

Yeah, it does look wrong.

> Tested manually with cross binutils for ppc64-linux-gnu and
> ppc64-vxworks7r2.

"Tested manually", what does that mean?  You compiled <something> <some
way> with and without the patch, and looked at the eventual machine code
<some way>, and <something changed>.  Please fill in the blanks.

Or "<something> didn't work but now it does".  Whatever will give us the
warm fuzzies and make us feel confident your patch is an improvement
over the status quo.

> Unless there's any objection in the next few days, I
> plan to put this in along with other changes.

You cannot do that, ever.

The patch itself looks fine, but the commit message doesn't, as said,
and the changelog not either:

> for  gcc/ChangeLog
> 
> 	* configure.ac: Adjust base register in linker test for large
> 	toc support.
> 	* configure: Rebuilt.

You should never use passive tense in changelogs.  Oh, and changelog
lines are 80 character positions long, please don't wrap early.  And
"TOC" is an initialism, spelled in all capitals.


Segher
Alexandre Oliva Sept. 14, 2025, 12:58 a.m. UTC | #2 
On Sep 13, 2025, Segher Boessenkool <segher@kernel.crashing.org> wrote:

> Hi!
> On Sat, Sep 13, 2025 at 12:07:33AM -0300, Alexandre Oliva wrote:

>> Tested manually with cross binutils for ppc64-linux-gnu and
>> ppc64-vxworks7r2.

> "Tested manually", what does that mean?

It means I fed the fragments (before and after) to the assembler and
linker, as run in that code path in configure, and got the same results
(the warning used to be discarded).

Given the same results, there's no functional change to the compiler.
I'm only removing a source of confusion.

I've also ran various builds with that patch in, in case that matters
for such a trivial change.  Even ppc targets.

>> Unless there's any objection in the next few days, I
>> plan to put this in along with other changes.

> You cannot do that, ever.

Of course I can.  I'm a co-maintainer of the modified files, and I have
been for almost 3 decades.  Even if I weren't, the error and the fix are
pretty obvious for anyone acquainted with ppc codegen, register
assignments and relocations.

> The patch itself looks fine, but the commit message doesn't

I don't see anything wrong with the single-paragraph commit message, and
you seemed to agree with it.  What are you getting at?  Are you by any
chance under the mistaken notion that the description of how I tested
the patch and when I intended to commit it was part of a commit message?

> and the changelog not either:

>> for  gcc/ChangeLog
>> 
>> * configure.ac: Adjust base register in linker test for large
>> toc support.
>> * configure: Rebuilt.

> You should never use passive tense in changelogs.

I'm familiar with that theory.  Practice has had the tense of this
specific verb split about evenly in the top-level Changelog.  But since
you seem to feel strongly about it, I'll adjust the spelling.

> Oh, and changelog
> lines are 80 character positions long, please don't wrap early.

The top-level ChangeLog says 76; gcc/ChangeLog doesn't override the
default.  Why does that matter, it's not even like we'd save a line or
anything...  Also, there are plenty of commit messages that don't go
past the earlier 70-columns standard.  When and how did it change,
anyway?

I'd even argue that it's good not to use too-long lines in ChangeLog
entries that go in the commit log, because git indents that, so they'd
go past the 80-columns standard terminal width.  I feel pretty strongly
about that myself, so I'm not changing that.

> And "TOC" is an initialism, spelled in all capitals.

I don't usually care much about capitalization, and I'm sure I can find
plenty of examples of ppc's ToC spelled with various different
capitalizations, but if you feel strongly about it, I don't mind
tweaking it to your liking (done), provided that nobody else objects to
the alternative.


Here's what the relevant part of the git log output now looks like:

    [ppc] adjust configure test for large TOC support
    
    The use of the TLS register in a TOC/GOT address computation was
    probably a cut&pasto or a thinko.  It causes a linker warning and,
    because the TLS access in the test is incomplete, may cause
    significant confusion.  Adjust to use the TOC/GOT register as base.
    
    
    for  gcc/ChangeLog
    
            * configure.ac: Adjust base register in linker test for large
            TOC support.
            * configure: Rebuild.
diff mbox series

Patch

diff --git a/gcc/configure b/gcc/configure
index 5a779db0a29f5..d6cc7fc17ca04 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -33209,7 +33209,7 @@  ie0:	.space 8
 	.global _start
 	.text
 _start:
-	addis 9,13,ie0@got@tprel@ha
+	addis 9,2,ie0@got@tprel@ha
 	ld 9,ie0@got@tprel@l(9)
 EOF
       if $gcc_cv_as -a64 -o conftest.o conftest.s > /dev/null 2>&1 \
diff --git a/gcc/configure.ac b/gcc/configure.ac
index 7e57d527ecdde..19975fa5be5b2 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -6789,7 +6789,7 @@  ie0:	.space 8
 	.global _start
 	.text
 _start:
-	addis 9,13,ie0@got@tprel@ha
+	addis 9,2,ie0@got@tprel@ha
 	ld 9,ie0@got@tprel@l(9)
 EOF
       if $gcc_cv_as -a64 -o conftest.o conftest.s > /dev/null 2>&1 \