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