Re: [PATCH 1/8] newlib: internalize HAVE_INITFINI_ARRAY

On Tue, 18 Jan 2022 at 19:53, Mike Frysinger <[email protected]> wrote:

> On 18 Jan 2022 11:10, C Howland wrote:
> > > ------------------------------
> > > *From:* Newlib <[email protected]>
> on
> > > behalf of Mike Frysinger <[email protected]>
> > > *Sent:* Monday, January 17, 2022 11:47 PM
> > > *To:* [email protected] <[email protected]>
> > > *Subject:* [PATCH 1/8] newlib: internalize HAVE_INITFINI_ARRAY
> > >
> > > This define is only used by newlib internally, so stop exporting it
> > > as HAVE_INITFINI_ARRAY since this can conflict with defines packages
> > > use themselves.
> > >
> > > We don't really need to add _ to HAVE_INIT_FINI too since it isn't
> > > exported in newlib.h, but might as well be consistent here.
> >
> >      How do you know it is only used by Newlib internally?  Changing this
> > is effectively changing the API and is not safe.  (I don't use it,
> myself,
> > but given it's been like this for a long time, there's nothing to say
> that
> > nobody is.)  Unfortunately, it uses a methodology as either being defined
> > or not, so someone using the #ifdef method on it would not immediately
> > notice the change. (That is, when building with something that looked at
> > it, the build itself could not know something had gone wrong.  It would
> > require runtime to find out.)
> >      This does not necessarily mean that this specific change ought not
> be
> > done, but it does mean that this consideration needs to be weighed before
> > an API-breaking change were made.  Offhand I can't think of a good way to
> > guard against it, either.  (A tedious way would be to mark it deprecated
> > and then remove it in a year or so.)
>
> any project relying on this is broken.  exporting this symbol violates
> standards
> as to the namespace C libraries are allowed to use.  in fact, a cursory
> search
> shows that it is breaking projects because they were using this define,
> but the
> newlib symbol override their configure tests leading to desyncs.
>
> we also don't export HAVE_INIT_FINI, so newlib users aren't able to
> determine
> how newlib is actually going to behave at run time.
>
> there is no way to mark a preprocessor symbol as deprecated such that the
> toolchain will warn.  we can put a note in the docs/NEWS, but that requires
> people actually read them.  and if they're reading those, then we can just
> as easily put mention that the symbol has been removed.
>
> i really don't think this is a big deal.  arguing theory isn't useful here.
>
> if an actual user comes out of the woodworks, we can consider whether it's
> worth adding it, and in the right namespace.
> -mike
>

     There is no exporting of HAVE_INIT_FINI as it is a preprocessor
define.  All the user has to do it to include newlib.h and they see it (or
fail to see it).  (I think we might have a word-choice difference here.
It's a preprocessor define, and is only of local file scope at
preprocessing time.  You can't "export" the variable to another file, it
only comes directly from newlib.h.)  But this is unrelated to the primary
point I'm making.
     Yes, agreed, it is violating preprocessor namespace.
     I'm not arguing just theory.  I am pointing out that this changes API
(albeit at the preprocessor level instead of the more usual linkage level)
and we are always very careful when API is changed.  If this case is one in
which changing it is OK, fine.  We just need to consciously make that
decision; my intent in writing is only that we directly make the choice
instead of falling sideways into it.  (You clearly think this one is worth
it.  I happen to agree, but that's not that important because it's
generally practice for Corinna or Jeff to OK this class of change.)
                         Craig