RE: [PATCH v2] libgloss: Resolve compilation errors for mips.

 

Hi Jeff,

Awesome!  Very many thanks for taking care of this.

I can confirm that with your commit, it’s once again possible to

build a “combined” source tree including gcc, binutils and newlib

for both “—target=mips-elf” and “—target=mips64-elf”.

 

Thank you again,

Roger

--

 

From: Jeff Johnston <[email protected]> 
Sent: 19 February 2025 20:29
To: Roger Sayle <[email protected]>
Cc: [email protected]; Mike Frysinger <[email protected]>
Subject: Re: [PATCH v2] libgloss: Resolve compilation errors for mips.

 

I have modified the patch and will merge.  See below for comments about suggested changes from Corinna.  I also

implemented one of Mike's comments regarding ptrdiff_t which wasn't implemented in the new patch.  I don't have

a mips toolset to test with so if there are any build issues, just let me know and we'll fix them.

 

-- Jeff J

 

On Wed, Feb 19, 2025 at 2:18 AM Roger Sayle <[email protected] <mailto:[email protected]> > wrote:


> On Feb 18 14:06, Roger Sayle wrote:
> > As requested here's the same patch as generated by "git format-patch -1 HEAD"
> > instead of the previous "git diff" format patch.  p.s. either format
> > can be applied to a clean source tree using the command "patch -p1 <
> filename.patch".
> 
> I'm aware of that, but newlib-cygwin is a git repo since 2016.
> Not using git-formatted patches moves the onus for correct author information as
> well as correct content of the commit message from the author to the commiter.
> So I hope it's ok to request git-formated patches.  A detailed commit message
> doesn't hurt either.

GCC maintainers still use (prefer?) "git diff" and CVS-style commit messages.
The mips-elf parts of GCC (and binutils) compile, the mips-elf parts of newlib don't.

> > --- a/libgloss/mips/cfe_mem.c
> > +++ b/libgloss/mips/cfe_mem.c
> > @@ -47,7 +47,7 @@ void *get_mem_info (struct s_mem *);  extern char
> > _end[];
> >
> >  /* Address immediately after available memory.  */ -static unsigned
> > long memtop;
> > +static void *memtop;
> >
> >  /* Program stack size.  */
> >  static unsigned long stack_size;
> 
> Shouldn't this be changed to size_t?

Wouldn't this change/break the ABI when size_t isn't a 32-bit unsigned long?

 

Yes, but I believe it should be unsigned long looking at the type headers.  The stack_size is used in calculations of

the s_mem struct size field which is declared as unsigned int.  I think for the time-being this

can be left as-is.

 


> > @@ -107,7 +107,7 @@ __libcfe_meminit (void)
> >    /* Chop the top of memory to a 32-byte aligned location, and
> >       round the stack size up to a 32-byte multiple.  */
> > -  memtop = memtop & ~(unsigned long)31;
> > +  memtop = (void*)((unsigned long)memtop & ~(unsigned long)31);
> 
> Shouldn't unsigned long be changed to uintptr_t here, too?

 

Changed to uintptr_t.  

 

Likewise.
> 
> >    stack_size = (stack_size + 31) & ~(unsigned long)31;
> 
> And size_t here?
Likewise.

 

Leaving as-is due to the s_mem struct.

 


The goal is not to change the code's behavior (even to fix bugs) or make
this code more portable.  But purely to allow it to build with a post K&R
compiler, and by restoring libgloss, allow GCC cross-compilers for mips-elf 
to be built/tested.  Without access to the affected hardware, I'm hesitant
to make changes that I can't test.  Hence as the current libcfe API returns 
an unsigned long, I continue to do the same with my patch.
Please feel free to submit your own follow-up patches.

If the code wasn't already so badly broken that it doesn't compile
I/someone wouldn't have to fix it.   I've already left it over a year 
since originally submitting this patch to see if a newlib maintainer
would attempt a fix using uintptr_t or size_t themselves.
Please, please feel free to fix this yourselves.

I'm happy to ping again next year.
Many thanks in advance,
Roger
--