Re: [PATCH 1/4] perf ui progress: Make sure we always define step value

[Milian Wolff]

On Freitag, 8. September 2017 15:26:42 CEST Arnaldo Carvalho de Melo wrote:
> Em Fri, Sep 08, 2017 at 03:16:37PM +0200, Milian Wolff escreveu:
> > On Freitag, 8. September 2017 14:05:07 CEST Jiri Olsa wrote:
> > > +++ b/tools/perf/ui/progress.c
> > > @@ -25,7 +25,7 @@ void ui_progress__update(struct ui_progress *p, u64
> > > adv)
> > > 
> > >  void ui_progress__init(struct ui_progress *p, u64 total, const char
> > >  *title)
> > > 
> > > {
> > > 
> > >   p->curr = 0;
> > > 
> > > - p->next = p->step = total / 16;
> > > + p->next = p->step = total / 16 ?: 1;
> > > 
> > >   p->total = total;
> > >   p->title = title;
> > 
> > This is a GNU extension, does this compile with clang?
> 
> Huh?
> 
> [acme@jouet linux]$ find tools/ -name "*.[ch]"| xargs grep ?: | wc -l
> 64
> [acme@jouet linux]$ find . -name "*.[ch]"| xargs grep ?: | wc -l
> 725
> [acme@jouet linux]$
> 
> And yes, tools/perf/ is built with clang regularly, I use containers to
> test build the tools/{perf,lib} codebase with gcc and clang on almost
> all distros, things like:

OK, thanks for the clarification. I was wondering because I didn't know about 
this syntax. Googling I found this: https://en.wikipedia.org/wiki/%3F:#C

Quote:

> A GNU extension to C allows omitting the second operand, and using 
implicitly the first operand as the second also:
>
> a = x ? : y;

Cheers

-- 
Milian Wolff | milian.wo...@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts

Re: [PATCH 1/4] perf ui progress: Make sure we always define step value

[Milian Wolff]

On Freitag, 8. September 2017 15:26:42 CEST Arnaldo Carvalho de Melo wrote:
> Em Fri, Sep 08, 2017 at 03:16:37PM +0200, Milian Wolff escreveu:
> > On Freitag, 8. September 2017 14:05:07 CEST Jiri Olsa wrote:
> > > +++ b/tools/perf/ui/progress.c
> > > @@ -25,7 +25,7 @@ void ui_progress__update(struct ui_progress *p, u64
> > > adv)
> > > 
> > >  void ui_progress__init(struct ui_progress *p, u64 total, const char
> > >  *title)
> > > 
> > > {
> > > 
> > >   p->curr = 0;
> > > 
> > > - p->next = p->step = total / 16;
> > > + p->next = p->step = total / 16 ?: 1;
> > > 
> > >   p->total = total;
> > >   p->title = title;
> > 
> > This is a GNU extension, does this compile with clang?
> 
> Huh?
> 
> [acme@jouet linux]$ find tools/ -name "*.[ch]"| xargs grep ?: | wc -l
> 64
> [acme@jouet linux]$ find . -name "*.[ch]"| xargs grep ?: | wc -l
> 725
> [acme@jouet linux]$
> 
> And yes, tools/perf/ is built with clang regularly, I use containers to
> test build the tools/{perf,lib} codebase with gcc and clang on almost
> all distros, things like:

OK, thanks for the clarification. I was wondering because I didn't know about 
this syntax. Googling I found this: https://en.wikipedia.org/wiki/%3F:#C

Quote:

> A GNU extension to C allows omitting the second operand, and using 
implicitly the first operand as the second also:
>
> a = x ? : y;

Cheers

-- 
Milian Wolff | milian.wo...@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts

Re: [PATCH 1/4] perf ui progress: Make sure we always define step value

[Arnaldo Carvalho de Melo]

Em Fri, Sep 08, 2017 at 03:16:37PM +0200, Milian Wolff escreveu:
> On Freitag, 8. September 2017 14:05:07 CEST Jiri Olsa wrote:
> > +++ b/tools/perf/ui/progress.c
> > @@ -25,7 +25,7 @@ void ui_progress__update(struct ui_progress *p, u64 adv)
> >  void ui_progress__init(struct ui_progress *p, u64 total, const char *title)
> > {
> > p->curr = 0;
> > -   p->next = p->step = total / 16;
> > +   p->next = p->step = total / 16 ?: 1;
> > p->total = total;
> > p->title = title;
> 
> This is a GNU extension, does this compile with clang?

Huh?

[acme@jouet linux]$ find tools/ -name "*.[ch]"| xargs grep ?: | wc -l
64
[acme@jouet linux]$ find . -name "*.[ch]"| xargs grep ?: | wc -l
725
[acme@jouet linux]$

And yes, tools/perf/ is built with clang regularly, I use containers to
test build the tools/{perf,lib} codebase with gcc and clang on almost
all distros, things like:

[acme@jouet linux]$ cat ~/git/linux-perf-tools-build/fedora/26/Dockerfile 
# docker.io/acmel/linux-perf-tools-build-fedora:26
FROM docker.io/fedora:26
MAINTAINER Arnaldo Carvalho de Melo 
# The second dnf line is to be able to build a kernel, do a make 
header_install, etc,
# So that we can build samples/bpf/ and the kernel
RUN dnf -y install make gcc flex bison elfutils-libelf-devel \
   bc findutils clang llvm openssl git \
   elfutils-devel libunwind-devel audit-libs-devel \
   openssl-devel slang-devel gtk2-devel perl-ExtUtils-Embed \
   python-devel binutils-devel xz-devel numactl-devel 
systemtap-sdt-devel \
   redhat-rpm-config && \
dnf -y clean all && \
rm -rf /usr/share/doc /usr/share/gtk-doc /usr/share/locale /usr/share/man 
&& \
mkdir -m 777 -p /tmp/build/perf /tmp/build/objtool /tmp/build/linux && \
groupadd -r perfbuilder && \
useradd -m -r -g perfbuilder perfbuilder && \
chown -R perfbuilder.perfbuilder /tmp/build
USER perfbuilder
ENTRYPOINT make -C /git/linux/tools/perf O=/tmp/build/perf && \
   rm -rf /tmp/build/perf/{.[^.]*,*} && \
   make -C /git/linux/tools/perf O=/tmp/build/perf CC=clang && \
   rm -rf /tmp/build/perf/{.[^.]*,*} && \
   make NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf && \
   rm -rf /tmp/build/perf/{.[^.]*,*} && \
   make NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang 
&& \
   make -C /git/linux/tools/objtool O=/tmp/build/objtool && \
   make -C /git/linux O=/tmp/build/linux defconfig && \
   make -C /git/linux O=/tmp/build/linux headers_install && \
   make -C /git/linux O=/tmp/build/linux samples/bpf/
[acme@jouet linux]$

- Arnaldo

Re: [PATCH 1/4] perf ui progress: Make sure we always define step value

[Arnaldo Carvalho de Melo]

Em Fri, Sep 08, 2017 at 03:16:37PM +0200, Milian Wolff escreveu:
> On Freitag, 8. September 2017 14:05:07 CEST Jiri Olsa wrote:
> > +++ b/tools/perf/ui/progress.c
> > @@ -25,7 +25,7 @@ void ui_progress__update(struct ui_progress *p, u64 adv)
> >  void ui_progress__init(struct ui_progress *p, u64 total, const char *title)
> > {
> > p->curr = 0;
> > -   p->next = p->step = total / 16;
> > +   p->next = p->step = total / 16 ?: 1;
> > p->total = total;
> > p->title = title;
> 
> This is a GNU extension, does this compile with clang?

Huh?

[acme@jouet linux]$ find tools/ -name "*.[ch]"| xargs grep ?: | wc -l
64
[acme@jouet linux]$ find . -name "*.[ch]"| xargs grep ?: | wc -l
725
[acme@jouet linux]$

And yes, tools/perf/ is built with clang regularly, I use containers to
test build the tools/{perf,lib} codebase with gcc and clang on almost
all distros, things like:

[acme@jouet linux]$ cat ~/git/linux-perf-tools-build/fedora/26/Dockerfile 
# docker.io/acmel/linux-perf-tools-build-fedora:26
FROM docker.io/fedora:26
MAINTAINER Arnaldo Carvalho de Melo 
# The second dnf line is to be able to build a kernel, do a make 
header_install, etc,
# So that we can build samples/bpf/ and the kernel
RUN dnf -y install make gcc flex bison elfutils-libelf-devel \
   bc findutils clang llvm openssl git \
   elfutils-devel libunwind-devel audit-libs-devel \
   openssl-devel slang-devel gtk2-devel perl-ExtUtils-Embed \
   python-devel binutils-devel xz-devel numactl-devel 
systemtap-sdt-devel \
   redhat-rpm-config && \
dnf -y clean all && \
rm -rf /usr/share/doc /usr/share/gtk-doc /usr/share/locale /usr/share/man 
&& \
mkdir -m 777 -p /tmp/build/perf /tmp/build/objtool /tmp/build/linux && \
groupadd -r perfbuilder && \
useradd -m -r -g perfbuilder perfbuilder && \
chown -R perfbuilder.perfbuilder /tmp/build
USER perfbuilder
ENTRYPOINT make -C /git/linux/tools/perf O=/tmp/build/perf && \
   rm -rf /tmp/build/perf/{.[^.]*,*} && \
   make -C /git/linux/tools/perf O=/tmp/build/perf CC=clang && \
   rm -rf /tmp/build/perf/{.[^.]*,*} && \
   make NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf && \
   rm -rf /tmp/build/perf/{.[^.]*,*} && \
   make NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf CC=clang 
&& \
   make -C /git/linux/tools/objtool O=/tmp/build/objtool && \
   make -C /git/linux O=/tmp/build/linux defconfig && \
   make -C /git/linux O=/tmp/build/linux headers_install && \
   make -C /git/linux O=/tmp/build/linux samples/bpf/
[acme@jouet linux]$

- Arnaldo

Re: [PATCH 1/4] perf ui progress: Make sure we always define step value

[Milian Wolff]

On Freitag, 8. September 2017 14:05:07 CEST Jiri Olsa wrote:
> Unlikely, but we could have ui_progress__init being called
> with total < 16, which would set the next and step variables
> to 0. That would force unnecessary ui_progress__ops->update
> calls because 'next' would never raise.
> 
> Forcing the next and step values to be always > 0.
> 
> Link: http://lkml.kernel.org/n/tip-0zcvhmsmbhkfgoi7c670r...@git.kernel.org
> Signed-off-by: Jiri Olsa 
> ---
>  tools/perf/ui/progress.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/ui/progress.c b/tools/perf/ui/progress.c
> index a0f24c7115c5..a9c15804b1f6 100644
> --- a/tools/perf/ui/progress.c
> +++ b/tools/perf/ui/progress.c
> @@ -25,7 +25,7 @@ void ui_progress__update(struct ui_progress *p, u64 adv)
>  void ui_progress__init(struct ui_progress *p, u64 total, const char *title)
> {
>   p->curr = 0;
> - p->next = p->step = total / 16;
> + p->next = p->step = total / 16 ?: 1;
>   p->total = total;
>   p->title = title;

This is a GNU extension, does this compile with clang?

Cheers

-- 
Milian Wolff | milian.wo...@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts

Re: [PATCH 1/4] perf ui progress: Make sure we always define step value

[Milian Wolff]

On Freitag, 8. September 2017 14:05:07 CEST Jiri Olsa wrote:
> Unlikely, but we could have ui_progress__init being called
> with total < 16, which would set the next and step variables
> to 0. That would force unnecessary ui_progress__ops->update
> calls because 'next' would never raise.
> 
> Forcing the next and step values to be always > 0.
> 
> Link: http://lkml.kernel.org/n/tip-0zcvhmsmbhkfgoi7c670r...@git.kernel.org
> Signed-off-by: Jiri Olsa 
> ---
>  tools/perf/ui/progress.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/ui/progress.c b/tools/perf/ui/progress.c
> index a0f24c7115c5..a9c15804b1f6 100644
> --- a/tools/perf/ui/progress.c
> +++ b/tools/perf/ui/progress.c
> @@ -25,7 +25,7 @@ void ui_progress__update(struct ui_progress *p, u64 adv)
>  void ui_progress__init(struct ui_progress *p, u64 total, const char *title)
> {
>   p->curr = 0;
> - p->next = p->step = total / 16;
> + p->next = p->step = total / 16 ?: 1;
>   p->total = total;
>   p->title = title;

This is a GNU extension, does this compile with clang?

Cheers

-- 
Milian Wolff | milian.wo...@kdab.com | Senior Software Engineer
KDAB (Deutschland) GmbH KG, a KDAB Group company
Tel: +49-30-521325470
KDAB - The Qt Experts

[PATCH 1/4] perf ui progress: Make sure we always define step value

[Jiri Olsa]

Unlikely, but we could have ui_progress__init being called
with total < 16, which would set the next and step variables
to 0. That would force unnecessary ui_progress__ops->update
calls because 'next' would never raise.

Forcing the next and step values to be always > 0.

Link: http://lkml.kernel.org/n/tip-0zcvhmsmbhkfgoi7c670r...@git.kernel.org
Signed-off-by: Jiri Olsa 
---
 tools/perf/ui/progress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/ui/progress.c b/tools/perf/ui/progress.c
index a0f24c7115c5..a9c15804b1f6 100644
--- a/tools/perf/ui/progress.c
+++ b/tools/perf/ui/progress.c
@@ -25,7 +25,7 @@ void ui_progress__update(struct ui_progress *p, u64 adv)
 void ui_progress__init(struct ui_progress *p, u64 total, const char *title)
 {
p->curr = 0;
-   p->next = p->step = total / 16;
+   p->next = p->step = total / 16 ?: 1;
p->total = total;
p->title = title;
 
-- 
2.9.5

[PATCH 1/4] perf ui progress: Make sure we always define step value

[Jiri Olsa]

Unlikely, but we could have ui_progress__init being called
with total < 16, which would set the next and step variables
to 0. That would force unnecessary ui_progress__ops->update
calls because 'next' would never raise.

Forcing the next and step values to be always > 0.

Link: http://lkml.kernel.org/n/tip-0zcvhmsmbhkfgoi7c670r...@git.kernel.org
Signed-off-by: Jiri Olsa 
---
 tools/perf/ui/progress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/ui/progress.c b/tools/perf/ui/progress.c
index a0f24c7115c5..a9c15804b1f6 100644
--- a/tools/perf/ui/progress.c
+++ b/tools/perf/ui/progress.c
@@ -25,7 +25,7 @@ void ui_progress__update(struct ui_progress *p, u64 adv)
 void ui_progress__init(struct ui_progress *p, u64 total, const char *title)
 {
p->curr = 0;
-   p->next = p->step = total / 16;
+   p->next = p->step = total / 16 ?: 1;
p->total = total;
p->title = title;
 
-- 
2.9.5