Re: [dpdk-dev] Using _XOPEN_SOURCE macros may break builds on FreeBSD

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Adrien,

> On Tue, May 14, 2019 at 08:58:42AM +0000, Smoczynski, MarcinX wrote:
> > > > Hey Konstantin,
> > > >
> > > > On Mon, May 13, 2019 at 10:49:00AM +0000, Ananyev, Konstantin wrote:
> > > > > Hi Adrien,
> > > > >
> > > > > >
> > > > > > On Mon, May 13, 2019 at 09:51:24AM +0000, Smoczynski, MarcinX
> > > wrote:
> > > > > > > 10/05/2019 20:17, Thomas Monjalon:
> > > > > > > > 10/05/2019 19:14, Smoczynski, MarcinX:
> > > > > > > > > To summarize we have different visibility sets for Linux and
> > > > > > > > > BSD when using XOPEN_SOURCE or POSIX_C_SOURCE explicitly.
> > > To
> > > > > > > > > overcome this situation we can either remove problematic
> > > > > > > > > XOPEN macros from mk/meson rules (drivers/net/failsafe,
> > > > > > > > > drivers/net/mlx4,
> > > > > > > > > drivers/net/mlx5)
> > > > > > > >
> > > > > > > > What is the consequence of removing these macros in mlx and
> > > failsafe PMDs?
> > > > > > >
> > > > > > > The purpose of these *_SOURCE constants is to enable particular
> > > > > > > feature sets visibility. As long as we have GNU_SOURCE on Linux
> > > > > > > removing it won't have any consequences. On BSD it will unify
> > > > > > > feature sets visibility with the rest of sources. Can't think of any
> > > downsides here.
> > > > > > >
> > > > > > > I believe XOPEN_SOURCE was introduced to extend features not to
> > > restrict them.
> > > > > >
> > > > > > I confirm that under Linux, all IPPROTO_* (POSIX/XOPEN/RFC1700)
> > > > > > are defined regardless (_GNU_SOURCE not even needed), while under
> > > > > > FreeBSD, the non-POSIX versions are only defined when
> > > __BSD_VISIBLE is set.
> > > > > >
> > > > > > The FreeBSD behavior is more correct in this respect since the
> > > > > > purpose of _XOPEN_SOURCE and friends is also to let applications
> > > > > > limit the risk of redefinitions in case they were written for an
> > > > > > earlier standard (e.g. -D_XOPEN_SOURCE=500 vs. -
> > > D_XOPEN_SOURCE=600).
> > > > >
> > > > > Still not sure why do you need it for failsafe and mlx PMDs?
> > > > > Would something in these PMDs be broken without  '-
> > > D_XOPEN_SOURCE=600'?
> > > >
> > > > Well, not really. At least not anymore if they compile fine without it
> > > > on all supported targets. I don't mind if they are removed from PMDs.
> > > >
> > > > _XOPEN_SOURCE=600 was originally added to mlx4 (later inherited by
> > > > mlx5 and
> > > > failsafe) for the following reasons:
> > > >
> > > > - Out fo habit, since a lot of stuff in unistd.h and fcntl.h depends on it
> > > >   to be exposed. Some affected definitions were likely needed at some
> > > point.
> > > >
> > > > - Besides toggling C syntax extensions, forcing a C standard through the
> > > >   -std parameter (e.g. -std=c99) in order to guarantee a minimum level of
> > > >   C compliance disables the implicit presence of nonstandard definitions,
> > > >   which must be re-enabled as needed through the appropriate #defines.
> > > >
> > > > For instance, including unistd.h for getsid() stops working as soon as
> > > > you use -std=c99. On Linux you can get it back through -std=gnu99 or
> > > > by combining -std=c99 with -D_GNU_SOURCE or -D_XOPEN_SOURCE. The
> > > > latter was chosen because it is the standard define supposed to work
> > > across OSes.
> > > >
> > > > Historically mlx4 had to enable -std=c99 to be able to use various
> > > > features not present when GCC defaulted to -std=gnu90. It was later
> > > > transformed to
> > > > -std=c11 for similar reasons (anonymous members in structs/unions if
> > > > memory serves me right).
> > > >
> > > > > > DPDK applications may also define _XOPEN_SOURCE for their own
> > > > > > needs. They should still be able to use rte_ip.h afterward.
> > > > >
> > > > > I suppose they can, they would just have (on FreeBSD) to add '-D
> > > __BSD_VISIBLE'
> > > > > themselves.
> > > >
> > > > Of course, but public headers should be as self sufficient as possible.
> > > > Unless they provide really insane compiler flags, if user applications
> > > > get compilation errors after including some header we install on the
> > > > system, I think the blame is on DPDK.
> > > >
> > > > > > I think this reason is
> > > > > > enough to go with -D__BSD_VISIBLE under FreeBSD without removing
> > > > > > _XOPEN_SOURCE, as it should work regardless.
> > > > >
> > > > > So do you suggest to add '-D __BSD_VISIBLE'  into mlx/failsafe PMDs
> > > > > Makefiles/meson.build, or ... ?
> > > >
> > > > Since headers of our public API potentially require it, it must be
> > > > defined globally (unlike _XOPEN_SOURCE which is only local to a few
> > > PMDs):
> > > > app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk,
> > > > alongside -D_GNU_SOURCE.
> > > >
> > > > Add it to mlx*/failsafe only if that's not enough. Just make sure
> > > > applications inherit this flag.
> > >
> > > Ok, to summarize, eyour suggestion is:
> > > 1. remove -D_XOPEN_SOURCE=... from mlx and failsafe PMDs.
> > > 2. add '-D __BSD_VISIBLE'  into top level make/meson files
> > > (app/meson.build, lib/meson.build, mk/target/generic/rte.vars.mk) Similar
> > > to what we doing for -DGNU_SOURCE.
> > >
> > > If I understand you correctly, then it sounds ok to me.
> > >
> > > >
> > > > > > Looking at the patch [1], I also think there's another, simpler approach:
> > > > > > unless really performance critical, defining
> > > > > > rte_ipv6_get_next_ext() in rte_net.c instead of a static inline in
> > > rte_ip.h should address this issue.
> > > > >
> > > > > It is performance critical, and I think that function call for each
> > > > > ext header is a way too expensive approach.
> > > > > Will prefer to keep that function inline.
> > > >
> > > > OK, a bit cumbersome but since we're heading this way [2], how about
> > > > defining our own instead of all the above?
> > > >
> > > >  #define RTE_IPPROTO_HOPOPTS 0
> > > >  #define RTE_IPPROTO_ROUTING 43
> > > >  ...
> > > >
> > > > Which could prove handy later as it appears Linux and FreeBSD don't
> > > > have the same set of available IPPROTO_* definitions.
> > > >
> > > > Thoughts?
> > > >
> > > > [2] "[RFC v2 00/14] prefix network structures"
> > > >     https://mails.dpdk.org/archives/dev/2019-April/129752.html
> > >
> > > Yep, that's definitely an option too.
> > > But if we going to replace all current references of IPPROTO_ inside DPDK to
> > > RTE_IPROTO_ - the change will be massive.
> > > And for sure it is out of scope of this patch series.
> > > That's probably need to be done after Olivier RFC will be in and should be
> > > subject of a separate patch series.
> > > Konstantin
> >
> > I agree that we need RTE_IPPROTO* macros but as Konstantin pointed out this
> > would be a huge change and we should do that on top of Oliver's work in
> > a separate patch set.
> >
> > I will propose a patch set with:
> > 1. Removed XOPEN_SOURCE macros as they are not needed anymore
> > 2. Added BSD_VISIBLE at the top of build system.
> 
> Actually I still suggest to leave the existing _XOPEN_SOURCE for users of
> -std=whatever, even if covered globally by _GNU_SOURCE and __BSD_VISIBLE.
> I think it's useful as a reminder that they did their homework since this is
> macro is itself standard.
> 

If you insist, I don't mind to keep it  - less changes for us,
again I am not a maintainer, nor a user of these PMDs.
Just don't to see much rationale behind it.
Ss I understand from your previous letters -
with global flags in place, it would build with -std=...
regardless do we have or not XOPEN_SOURCE=... in these PMDs or not.
Konstantin


> Regarding RTE_IPPROTO*, my suggestion wasn't to convert DPDK entirely, only
> to add the missing ones so far only needed by this patch. Given their values
> are defined by RFCs, they should be fully compatible and interchangeable
> with system definitions.
> 
> I'm fine with either approach in any case.
> 
> --
> Adrien Mazarguil
> 6WIND