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

Mike Frysinger <[email protected]> Tue, 18 Jan 2022 22:44:04 -0500
Newsgroups	gmane.comp.lib.newlib
Message-ID	<YeeJBOFCCJiHrKx5@vapier>
On 18 Jan 2022 21:39, C Howland wrote:
> On Tue, 18 Jan 2022 at 19:53, Mike Frysinger <[email protected]> wrote:
> > On 18 Jan 2022 11:10, C Howland wrote:
> > > > 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.
> 
>      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.

i think you got confused by my point.  my patch is about the HAVE_INITFINI_ARRAY
macro.  i followed up pointing out that newlib also leverages HAVE_INIT_FINI
internally, but doesn't make that state visible externally.  hence anyone who
tries to leverage HAVE_INITFINI_ARRAY is already getting an incomplete picture.

preprocessor directives create macros, not variables.  i understand that
"exporting" isn't a thing here, and isn't the same as "extern" or other C-
level storage/linkage.  i'm referring to general concepts, not to specific
standards language, and i'm being lazy and imprecise as a result.  i don't
think we need to get into a pointless semantic debate here.

>      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.)

i understand your points.  i disagree that removing this symbol will have any
meaningful impact, and that any code that might actually break deserves any
consideration for using something it shouldn't have in the first place.
-mike
signature.asc (application/pgp-signature, 833 B)