Question about ancient TARGET_CFLAGS in rules.mk?

[Christian Marangi]

On Sun, Jul 17, 2022 at 02:54:24AM -0700, Rosen Penev wrote:
> 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)
> Not even rejected. example:
> >https://patchwork.ozlabs.org/project/openwrt/patch/20220622185800.3156-1-rosenp@gmail.com/ and https://patchwork.ozlabs.org/project/openwrt/patch/20201225011158.35592-1-rosenp@gmail.com/
> 
> I've had others but retired them after figuring out they were never
> going to get merged.

I mean I know they are gigantic corner case where you can build an
entire house in the corner... But what are the drawbacks of such small
fix? The NULL check one for example seems pretty important... probably
won't ever happen... But now that I look about it, could be that it was
ignored as returning early from that function would create much bigger
problems?

> > > >
> > > > 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...
> >
> > > >
> > > > _______________________________________________
> > > > openwrt-devel mailing list
> > > > openwrt-devel at lists.openwrt.org
> > > > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
> >
> > --
> >         Ansuel

-- 
	Ansuel