| Message ID | 20191120042856.415854-4-rosenp@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] treewide: sys/poll to poll | expand |
On Wed, 20 Nov 2019 05:28:56 +0100, Rosen Penev wrote: > > If the progress bar somehow becomes negative, it ends up overwritting > tmp. Fixes this GCC warning: > > aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes > into a region of size 4 [-Wformat-overflow=] > 1747 | sprintf(tmp, "%02d%%", maxperc[c]); This is false-positive. The value passed there is guaranteed to be a positive integer at the calculation time. thanks, Takashi > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > aplay/aplay.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/aplay/aplay.c b/aplay/aplay.c > index 72fa567..9c5a11b 100644 > --- a/aplay/aplay.c > +++ b/aplay/aplay.c > @@ -54,6 +54,8 @@ > #include "formats.h" > #include "version.h" > > +#define ABS(a) (a) < 0 ? -(a) : (a) > + > #ifdef SND_CHMAP_API_VERSION > #define CONFIG_SUPPORT_CHMAP 1 > #endif > @@ -1741,7 +1743,7 @@ static void print_vu_meter_stereo(int *perc, int *maxperc) > line[bar_length + 6 + 1 + p] = '+'; > else > line[bar_length - p - 1] = '+'; > - if (maxperc[c] > 99) > + if (ABS(maxperc[c]) > 99) > sprintf(tmp, "MAX"); > else > sprintf(tmp, "%02d%%", maxperc[c]); > -- > 2.23.0 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
On Tue, Nov 19, 2019 at 10:26 PM Takashi Iwai <tiwai@suse.de> wrote: > > On Wed, 20 Nov 2019 05:28:56 +0100, > Rosen Penev wrote: > > > > If the progress bar somehow becomes negative, it ends up overwritting > > tmp. Fixes this GCC warning: > > > > aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes > > into a region of size 4 [-Wformat-overflow=] > > 1747 | sprintf(tmp, "%02d%%", maxperc[c]); > > This is false-positive. The value passed there is guaranteed to be a > positive integer at the calculation time. Sure. But best to silence GCC. It probably optimizes better this way. > > > thanks, > > Takashi > > > > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > > --- > > aplay/aplay.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/aplay/aplay.c b/aplay/aplay.c > > index 72fa567..9c5a11b 100644 > > --- a/aplay/aplay.c > > +++ b/aplay/aplay.c > > @@ -54,6 +54,8 @@ > > #include "formats.h" > > #include "version.h" > > > > +#define ABS(a) (a) < 0 ? -(a) : (a) > > + > > #ifdef SND_CHMAP_API_VERSION > > #define CONFIG_SUPPORT_CHMAP 1 > > #endif > > @@ -1741,7 +1743,7 @@ static void print_vu_meter_stereo(int *perc, int *maxperc) > > line[bar_length + 6 + 1 + p] = '+'; > > else > > line[bar_length - p - 1] = '+'; > > - if (maxperc[c] > 99) > > + if (ABS(maxperc[c]) > 99) > > sprintf(tmp, "MAX"); > > else > > sprintf(tmp, "%02d%%", maxperc[c]); > > -- > > 2.23.0 > > > > _______________________________________________ > > Alsa-devel mailing list > > Alsa-devel@alsa-project.org > > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel > >
On Wed, 20 Nov 2019 07:36:19 +0100, Rosen Penev wrote: > > On Tue, Nov 19, 2019 at 10:26 PM Takashi Iwai <tiwai@suse.de> wrote: > > > > On Wed, 20 Nov 2019 05:28:56 +0100, > > Rosen Penev wrote: > > > > > > If the progress bar somehow becomes negative, it ends up overwritting > > > tmp. Fixes this GCC warning: > > > > > > aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes > > > into a region of size 4 [-Wformat-overflow=] > > > 1747 | sprintf(tmp, "%02d%%", maxperc[c]); > > > > This is false-positive. The value passed there is guaranteed to be a > > positive integer at the calculation time. > Sure. But best to silence GCC. It probably optimizes better this way. I guess this adds more code in binary. Comparing with 99U would work? Takashi > > > > > > thanks, > > > > Takashi > > > > > > > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > > > --- > > > aplay/aplay.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/aplay/aplay.c b/aplay/aplay.c > > > index 72fa567..9c5a11b 100644 > > > --- a/aplay/aplay.c > > > +++ b/aplay/aplay.c > > > @@ -54,6 +54,8 @@ > > > #include "formats.h" > > > #include "version.h" > > > > > > +#define ABS(a) (a) < 0 ? -(a) : (a) > > > + > > > #ifdef SND_CHMAP_API_VERSION > > > #define CONFIG_SUPPORT_CHMAP 1 > > > #endif > > > @@ -1741,7 +1743,7 @@ static void print_vu_meter_stereo(int *perc, int *maxperc) > > > line[bar_length + 6 + 1 + p] = '+'; > > > else > > > line[bar_length - p - 1] = '+'; > > > - if (maxperc[c] > 99) > > > + if (ABS(maxperc[c]) > 99) > > > sprintf(tmp, "MAX"); > > > else > > > sprintf(tmp, "%02d%%", maxperc[c]); > > > -- > > > 2.23.0 > > > > > > _______________________________________________ > > > Alsa-devel mailing list > > > Alsa-devel@alsa-project.org > > > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel > > > >
On Tue, Nov 19, 2019 at 10:48 PM Takashi Iwai <tiwai@suse.de> wrote: > > On Wed, 20 Nov 2019 07:36:19 +0100, > Rosen Penev wrote: > > > > On Tue, Nov 19, 2019 at 10:26 PM Takashi Iwai <tiwai@suse.de> wrote: > > > > > > On Wed, 20 Nov 2019 05:28:56 +0100, > > > Rosen Penev wrote: > > > > > > > > If the progress bar somehow becomes negative, it ends up overwritting > > > > tmp. Fixes this GCC warning: > > > > > > > > aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes > > > > into a region of size 4 [-Wformat-overflow=] > > > > 1747 | sprintf(tmp, "%02d%%", maxperc[c]); > > > > > > This is false-positive. The value passed there is guaranteed to be a > > > positive integer at the calculation time. > > Sure. But best to silence GCC. It probably optimizes better this way. > > I guess this adds more code in binary. Comparing with 99U would work? I tried that. Here are some results: 134348 for this patch 134832 if changed to U. Also tried casting lhs to unnsigned int, same size. 135125 originally I understand this is an exercise in micro-optimization, but still. Sizes are in bytes. It's the size of a compressed OpenWrt archive. > > > Takashi > > > > > > > > > > > thanks, > > > > > > Takashi > > > > > > > > > > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > > > > --- > > > > aplay/aplay.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/aplay/aplay.c b/aplay/aplay.c > > > > index 72fa567..9c5a11b 100644 > > > > --- a/aplay/aplay.c > > > > +++ b/aplay/aplay.c > > > > @@ -54,6 +54,8 @@ > > > > #include "formats.h" > > > > #include "version.h" > > > > > > > > +#define ABS(a) (a) < 0 ? -(a) : (a) > > > > + > > > > #ifdef SND_CHMAP_API_VERSION > > > > #define CONFIG_SUPPORT_CHMAP 1 > > > > #endif > > > > @@ -1741,7 +1743,7 @@ static void print_vu_meter_stereo(int *perc, int *maxperc) > > > > line[bar_length + 6 + 1 + p] = '+'; > > > > else > > > > line[bar_length - p - 1] = '+'; > > > > - if (maxperc[c] > 99) > > > > + if (ABS(maxperc[c]) > 99) > > > > sprintf(tmp, "MAX"); > > > > else > > > > sprintf(tmp, "%02d%%", maxperc[c]); > > > > -- > > > > 2.23.0 > > > > > > > > _______________________________________________ > > > > Alsa-devel mailing list > > > > Alsa-devel@alsa-project.org > > > > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel > > > > > >
On Wed, Nov 20, 2019 at 10:34 AM Takashi Iwai <tiwai@suse.de> wrote: > > On Wed, 20 Nov 2019 18:55:43 +0100, > Rosen Penev wrote: > > > > On Tue, Nov 19, 2019 at 10:48 PM Takashi Iwai <tiwai@suse.de> wrote: > > > > > > On Wed, 20 Nov 2019 07:36:19 +0100, > > > Rosen Penev wrote: > > > > > > > > On Tue, Nov 19, 2019 at 10:26 PM Takashi Iwai <tiwai@suse.de> wrote: > > > > > > > > > > On Wed, 20 Nov 2019 05:28:56 +0100, > > > > > Rosen Penev wrote: > > > > > > > > > > > > If the progress bar somehow becomes negative, it ends up overwritting > > > > > > tmp. Fixes this GCC warning: > > > > > > > > > > > > aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes > > > > > > into a region of size 4 [-Wformat-overflow=] > > > > > > 1747 | sprintf(tmp, "%02d%%", maxperc[c]); > > > > > > > > > > This is false-positive. The value passed there is guaranteed to be a > > > > > positive integer at the calculation time. > > > > Sure. But best to silence GCC. It probably optimizes better this way. > > > > > > I guess this adds more code in binary. Comparing with 99U would work? > > I tried that. Here are some results: > > > > 134348 for this patch > > 134832 if changed to U. Also tried casting lhs to unnsigned int, same size. > > 135125 originally > > > > I understand this is an exercise in micro-optimization, but still. > > > > Sizes are in bytes. It's the size of a compressed OpenWrt archive. > > Thanks for the analysis. It's surprising, though, the original code > became bigger. I've learned not to question the compiler. If it complains, it means it generates suboptimal code. > > The cast of lhs is basically superfluous since C performs the cast > implicitly at comparison, BTW. > > In anyway, the number tells. Could you respin this patch? I can resend. Not sure what you really want. > Meanwhile I'll apply the rest patches. > > > thanks, > > Takashi
diff --git a/aplay/aplay.c b/aplay/aplay.c index 72fa567..9c5a11b 100644 --- a/aplay/aplay.c +++ b/aplay/aplay.c @@ -54,6 +54,8 @@ #include "formats.h" #include "version.h" +#define ABS(a) (a) < 0 ? -(a) : (a) + #ifdef SND_CHMAP_API_VERSION #define CONFIG_SUPPORT_CHMAP 1 #endif @@ -1741,7 +1743,7 @@ static void print_vu_meter_stereo(int *perc, int *maxperc) line[bar_length + 6 + 1 + p] = '+'; else line[bar_length - p - 1] = '+'; - if (maxperc[c] > 99) + if (ABS(maxperc[c]) > 99) sprintf(tmp, "MAX"); else sprintf(tmp, "%02d%%", maxperc[c]);
If the progress bar somehow becomes negative, it ends up overwritting tmp. Fixes this GCC warning: aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes into a region of size 4 [-Wformat-overflow=] 1747 | sprintf(tmp, "%02d%%", maxperc[c]); Signed-off-by: Rosen Penev <rosenp@gmail.com> --- aplay/aplay.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)