Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1577779 > unrolled thread

Intel PT decoder switch case fallthrough cases reported by gcc 7

Back to article view | Back to linux.kernel

Contents

  Intel PT decoder switch case fallthrough cases reported by gcc 7 Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-09 18:00 +0100
    Re: Intel PT decoder switch case fallthrough cases reported by gcc 7 Andi Kleen <andi@firstfloor.org> - 2017-02-09 19:50 +0100
      Re: Intel PT decoder switch case fallthrough cases reported by gcc 7 Andi Kleen <andi@firstfloor.org> - 2017-02-09 20:20 +0100
        Re: Intel PT decoder switch case fallthrough cases reported by gcc 7 Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-09 20:30 +0100
      Re: Intel PT decoder switch case fallthrough cases reported by gcc 7 Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-09 20:20 +0100

#1577779 — Intel PT decoder switch case fallthrough cases reported by gcc 7

Hi,

	I've updated the container with Fedora Rawhide I use to build
tools/perf/ and samples/bcc/ and it now comes with gcc 7, where I get
things like:

  CC       /tmp/build/perf/tests/code-reading.o
util/intel-pt-decoder/intel-pt-decoder.c: In function 'intel_pt_walk_psb':
util/intel-pt-decoder/intel-pt-decoder.c:1748:31: error: this statement may fall through [-Werror=implicit-fallthrough=]
    decoder->continuous_period = false;
                               ^
util/intel-pt-decoder/intel-pt-decoder.c:1749:3: note: here
   case INTEL_PT_TIP_PGE:
   ^~~~
util/intel-pt-decoder/intel-pt-decoder.c:1801:4: error: this statement may fall through [-Werror=implicit-fallthrough=]
    intel_pt_clear_tx_flags(decoder);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
util/intel-pt-decoder/intel-pt-decoder.c:1802:3: note: here
   case INTEL_PT_TNT:
   ^~~~
util/intel-pt-decoder/intel-pt-decoder.c: In function 'intel_pt_walk_to_ip':
util/intel-pt-decoder/intel-pt-decoder.c:1841:31: error: this statement may fall through [-Werror=implicit-fallthrough=]
    decoder->continuous_period = false;
                               ^
util/intel-pt-decoder/intel-pt-decoder.c:1842:3: note: here
   case INTEL_PT_TIP_PGE:
   ^~~~
  MKDIR    /tmp/build/perf/util/scripting-engines/


This gets solved with a new attribute, that you have to add where in the past
we added:

	/* Fall through */

To indicate that the fall through to the next case statement block is
intentional, so now I have a __fallthrough and I am addressing all the cases,
please check if the ones below are the ones intended for the Intel PT parts,
please Ack or advise, only one seemed like a bug, i.e. a break should be used,
but I'm not sure.

- Arnaldo


diff --git a/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c b/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c
index e4e7dc781d21..d4ed327a4908 100644
--- a/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c
+++ b/tools/perf/util/intel-pt-decoder/intel-pt-decoder.c
@@ -1746,6 +1746,7 @@ static int intel_pt_walk_psb(struct intel_pt_decoder *decoder)
 		switch (decoder->packet.type) {
 		case INTEL_PT_TIP_PGD:
 			decoder->continuous_period = false;
+			__fallthrough;
 		case INTEL_PT_TIP_PGE:
 		case INTEL_PT_TIP:
 			intel_pt_log("ERROR: Unexpected packet\n");
@@ -1799,6 +1800,8 @@ static int intel_pt_walk_psb(struct intel_pt_decoder *decoder)
 			decoder->pge = false;
 			decoder->continuous_period = false;
 			intel_pt_clear_tx_flags(decoder);
+			break;
+
 		case INTEL_PT_TNT:
 			decoder->have_tma = false;
 			intel_pt_log("ERROR: Unexpected packet\n");
@@ -1839,6 +1842,7 @@ static int intel_pt_walk_to_ip(struct intel_pt_decoder *decoder)
 		switch (decoder->packet.type) {
 		case INTEL_PT_TIP_PGD:
 			decoder->continuous_period = false;
+			__fallthrough;
 		case INTEL_PT_TIP_PGE:
 		case INTEL_PT_TIP:
 			decoder->pge = decoder->packet.type != INTEL_PT_TIP_PGD;
diff --git a/tools/perf/util/intel-pt-decoder/intel-pt-pkt-decoder.c b/tools/perf/util/intel-pt-decoder/intel-pt-pkt-decoder.c
index 4f7b32020487..7528ae4f7e28 100644
--- a/tools/perf/util/intel-pt-decoder/intel-pt-pkt-decoder.c
+++ b/tools/perf/util/intel-pt-decoder/intel-pt-pkt-decoder.c
@@ -17,6 +17,7 @@
 #include <string.h>
 #include <endian.h>
 #include <byteswap.h>
+#include <linux/compiler.h>
 
 #include "intel-pt-pkt-decoder.h"
 
@@ -498,6 +499,7 @@ int intel_pt_pkt_desc(const struct intel_pt_pkt *packet, char *buf,
 	case INTEL_PT_FUP:
 		if (!(packet->count))
 			return snprintf(buf, buf_len, "%s no ip", name);
+		__fallthrough;
 	case INTEL_PT_CYC:
 	case INTEL_PT_VMCS:
 	case INTEL_PT_MTC:

[toc] | [next] | [standalone]

#1577861

On Thu, Feb 09, 2017 at 01:50:39PM -0300, Arnaldo Carvalho de Melo wrote:
> Hi,
> 
> 	I've updated the container with Fedora Rawhide I use to build
> tools/perf/ and samples/bcc/ and it now comes with gcc 7, where I get
> things like:

FWIW, but it just shows that you should never ship software with -Werror
enabled. New compiler releases add new warnings, and you just randomly
break the build for users who use newer compilers.

It's ok to use in your own builds, but should never be default.

The kernel gets it right, but perf is wrong here.

-Andi

[toc] | [prev] | [next] | [standalone]

#1577887

> But what about my question? Do you think the changes are ok? I actually
> made all be fallthrough, i.e. considered that the existing code was ok.

Yes the changes are fine.

-Andi

[toc] | [prev] | [next] | [standalone]

#1577891

Em Thu, Feb 09, 2017 at 11:13:16AM -0800, Andi Kleen escreveu:
> > But what about my question? Do you think the changes are ok? I actually
> > made all be fallthrough, i.e. considered that the existing code was ok.
> 
> Yes the changes are fine.

Thanks!

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1577890

Em Thu, Feb 09, 2017 at 10:25:19AM -0800, Andi Kleen escreveu:
> On Thu, Feb 09, 2017 at 01:50:39PM -0300, Arnaldo Carvalho de Melo wrote:
> > Hi,
> > 
> > 	I've updated the container with Fedora Rawhide I use to build
> > tools/perf/ and samples/bcc/ and it now comes with gcc 7, where I get
> > things like:
> 
> FWIW, but it just shows that you should never ship software with -Werror
> enabled. New compiler releases add new warnings, and you just randomly
> break the build for users who use newer compilers.
> 
> It's ok to use in your own builds, but should never be default.
> 
> The kernel gets it right, but perf is wrong here.

But what about my question? Do you think the changes are ok? I actually
made all be fallthrough, i.e. considered that the existing code was ok.

- Arnaldo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web