Question about ancient TARGET_CFLAGS in rules.mk?

[Rosen Penev]

On Sun, Jul 17, 2022 at 2:29 AM Christian Marangi <ansuelsmth at gmail.com> wrote:
>
> On Sat, Jul 16, 2022 at 10:17:28PM -0700, Rosen Penev wrote:
> > On Sat, Jul 16, 2022 at 4:52 PM Ansuel Smith <ansuelsmth at gmail.com> wrote:
> > >
> > > Hi,
> > > some background about this.
> > >
> > > I'm trying to improve our CI system more and more by finally adding
> > > support for real
> > > EXTERNAL_TOOLCHAIN_SUPPORT... I'm running (and abusing) the github CI
> > > to make sure everything works and all compiles correctly...
> > >
> > > While testing it I notice a specific target fails to compile ubox package.
> > > While still to investigate why this is only present on that target, i
> > > found out why
> > > this happen with EXTERNAL_TOOLCHAIN and doesn't fail on a normal build.
> > >
> > > The error is this
> > >
> > > kmodloader.c: In function 'main_loader':
> > > 1339kmodloader.c:1027:41: error: ignoring return value of 'asprintf'
> > > declared with attribute 'warn_unused_result' [-Werror=unused-result]
> > > 1340make[1]: *** [package/Makefile:116: package/system/ubox/compile] Error 1
> > > 1341 1027 | asprintf(&m->opts, "%s %s", prev, opts);
> > > 1342 | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > 1343cc1: all warnings being treated as errors
> > >
> > > The package is compiled with -Wall so it does make sense that the
> > > error is printed...
> > >
> > > Fact is that the error(warning) is actually correct but I couldn't
> > > understand why it was
> > > not flagged on normal build and here the reason...
> > >
> > > In rules.mk we have
> > > TARGET_CFLAGS+= -fhonour-copts -Wno-error=unused-but-set-variable
> > > -Wno-error=unused-result
> > > and this is only applied if EXTERNAL_TOOLCHAIN is not selected...
> > >
> > > Now the question... WHY?
> > >
> > > Considering even the linux kernel started to use Wall by default,
> > > should we also start
> > > enforcing correct code and fix every package that present such error?
> > Yes
>
> Ok will prepare a patch.
>
> > > Fixing these kind of error is trivial enough and IMHO we should drop these
> > > warning disable.
> > I've had issues getting stuff merged to core openwrt utilities in the
> > past, especially when it comes to fixing compilation warnings.
>
> Mhhh I notice sometime patch getting rejected as it was trying to fix an
> false-positive error from a faulty version of gcc.
> But I think fixing error caused by disabling warning should be
> accepted... Real problem is that some trivial fix may cause problems...
> Example the error i just fixed for kmodloader... If I wasn't carfule i
> could totally check the error condition for (fail) instead of (fail < 0)
> and that would have caused breakage as asprintf return the bytes written
> so it could totally return a value != 0. (just an example of a simple
> error handling destryong the function of the package)
>
> > >
> > > I will create a PR in the next few days but wonder if anyone wants to
> > > give some feedback
> > > about why these extra flags are set. To me it seems they are just
> > > leftover from older times?
> > Most likely.
>
> Still can't understand why these errors are only in archs38/generic...
> Still a mistery to me...
very clear actually. glibc marks asprintf with
attribute((warn_unused_result)) (nodiscard in C++). musl does not.
>
> > >
> > > _______________________________________________
> > > openwrt-devel mailing list
> > > openwrt-devel at lists.openwrt.org
> > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
>
> --
>         Ansuel