| Message ID | c8a43b38b1eb348653bcbca2ebcbd0d796f01ac4.1544562436.git.sdf@google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] selftests/bpf: don't use bpf helpers in non-bpf environment | expand |
On 12/11, Stanislav Fomichev wrote: > We're using bpf_htons in test_progs.c to initialize some static > global data and I think I hit some weird case on an older compiler > which doesn't have __builtin_bswap16 (and __builtin_constant_p > expands to false). > > In this case I see: > error: implicit declaration of function '__builtin_bswap16' > > Let's explicitly use __constant_htons which should be exposed by the > linux/byteorder.h uapi header. Forgot to mention, that using simple htons produces the following: test_progs.c:54:17: error: braced-group within expression allowed only inside a function .eth.h_proto = htons(ETH_P_IP), > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- > tools/testing/selftests/bpf/test_progs.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > index 26f1fdf3e2bf..61593d319c0e 100644 > --- a/tools/testing/selftests/bpf/test_progs.c > +++ b/tools/testing/selftests/bpf/test_progs.c > @@ -51,10 +51,10 @@ static struct { > struct iphdr iph; > struct tcphdr tcp; > } __packed pkt_v4 = { > - .eth.h_proto = bpf_htons(ETH_P_IP), > + .eth.h_proto = __constant_htons(ETH_P_IP), > .iph.ihl = 5, > .iph.protocol = 6, > - .iph.tot_len = bpf_htons(MAGIC_BYTES), > + .iph.tot_len = __constant_htons(MAGIC_BYTES), > .tcp.urg_ptr = 123, > }; > > @@ -64,9 +64,9 @@ static struct { > struct ipv6hdr iph; > struct tcphdr tcp; > } __packed pkt_v6 = { > - .eth.h_proto = bpf_htons(ETH_P_IPV6), > + .eth.h_proto = __constant_htons(ETH_P_IPV6), > .iph.nexthdr = 6, > - .iph.payload_len = bpf_htons(MAGIC_BYTES), > + .iph.payload_len = __constant_htons(MAGIC_BYTES), > .tcp.urg_ptr = 123, > }; > > -- > 2.20.0.rc2.403.gdbc3b29805-goog >
On 12/11/2018 10:49 PM, Stanislav Fomichev wrote: > On 12/11, Stanislav Fomichev wrote: >> We're using bpf_htons in test_progs.c to initialize some static >> global data and I think I hit some weird case on an older compiler >> which doesn't have __builtin_bswap16 (and __builtin_constant_p >> expands to false). >> >> In this case I see: >> error: implicit declaration of function '__builtin_bswap16' Is that gcc < 4.8? >> Let's explicitly use __constant_htons which should be exposed by the >> linux/byteorder.h uapi header. > > Forgot to mention, that using simple htons produces the following: > test_progs.c:54:17: error: braced-group within expression allowed only > inside a function > .eth.h_proto = htons(ETH_P_IP), > >> Signed-off-by: Stanislav Fomichev <sdf@google.com> >> --- >> tools/testing/selftests/bpf/test_progs.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c >> index 26f1fdf3e2bf..61593d319c0e 100644 >> --- a/tools/testing/selftests/bpf/test_progs.c >> +++ b/tools/testing/selftests/bpf/test_progs.c >> @@ -51,10 +51,10 @@ static struct { >> struct iphdr iph; >> struct tcphdr tcp; >> } __packed pkt_v4 = { >> - .eth.h_proto = bpf_htons(ETH_P_IP), >> + .eth.h_proto = __constant_htons(ETH_P_IP), If the __builtin_constant_p() evaluated to false on the constants (?), wouldn't using the __bpf_constant_htons() directly work as well given it's not using a builtin either? Should be fine either way though using the same api/header might be slightly nicer. >> .iph.ihl = 5, >> .iph.protocol = 6, >> - .iph.tot_len = bpf_htons(MAGIC_BYTES), >> + .iph.tot_len = __constant_htons(MAGIC_BYTES), >> .tcp.urg_ptr = 123, >> }; >> >> @@ -64,9 +64,9 @@ static struct { >> struct ipv6hdr iph; >> struct tcphdr tcp; >> } __packed pkt_v6 = { >> - .eth.h_proto = bpf_htons(ETH_P_IPV6), >> + .eth.h_proto = __constant_htons(ETH_P_IPV6), >> .iph.nexthdr = 6, >> - .iph.payload_len = bpf_htons(MAGIC_BYTES), >> + .iph.payload_len = __constant_htons(MAGIC_BYTES), >> .tcp.urg_ptr = 123, >> }; >> >> -- >> 2.20.0.rc2.403.gdbc3b29805-goog >>
On 12/12, Daniel Borkmann wrote: > On 12/11/2018 10:49 PM, Stanislav Fomichev wrote: > > On 12/11, Stanislav Fomichev wrote: > >> We're using bpf_htons in test_progs.c to initialize some static > >> global data and I think I hit some weird case on an older compiler > >> which doesn't have __builtin_bswap16 (and __builtin_constant_p > >> expands to false). > >> > >> In this case I see: > >> error: implicit declaration of function '__builtin_bswap16' > > Is that gcc < 4.8? Yes. > >> Let's explicitly use __constant_htons which should be exposed by the > >> linux/byteorder.h uapi header. > > > > Forgot to mention, that using simple htons produces the following: > > test_progs.c:54:17: error: braced-group within expression allowed only > > inside a function > > .eth.h_proto = htons(ETH_P_IP), > > > >> Signed-off-by: Stanislav Fomichev <sdf@google.com> > >> --- > >> tools/testing/selftests/bpf/test_progs.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > >> index 26f1fdf3e2bf..61593d319c0e 100644 > >> --- a/tools/testing/selftests/bpf/test_progs.c > >> +++ b/tools/testing/selftests/bpf/test_progs.c > >> @@ -51,10 +51,10 @@ static struct { > >> struct iphdr iph; > >> struct tcphdr tcp; > >> } __packed pkt_v4 = { > >> - .eth.h_proto = bpf_htons(ETH_P_IP), > >> + .eth.h_proto = __constant_htons(ETH_P_IP), > > If the __builtin_constant_p() evaluated to false on the constants (?), > wouldn't using the __bpf_constant_htons() directly work as well given > it's not using a builtin either? Should be fine either way though using > the same api/header might be slightly nicer. I got it wrong, __builtin_constant_p() evaluates correctly, I played with it a bit. But for some reason it still complains about that branch that it doesn't take :-/ Using __bpf_constant_htons() is a good idea, I'll follow up with a v2. > >> .iph.ihl = 5, > >> .iph.protocol = 6, > >> - .iph.tot_len = bpf_htons(MAGIC_BYTES), > >> + .iph.tot_len = __constant_htons(MAGIC_BYTES), > >> .tcp.urg_ptr = 123, > >> }; > >> > >> @@ -64,9 +64,9 @@ static struct { > >> struct ipv6hdr iph; > >> struct tcphdr tcp; > >> } __packed pkt_v6 = { > >> - .eth.h_proto = bpf_htons(ETH_P_IPV6), > >> + .eth.h_proto = __constant_htons(ETH_P_IPV6), > >> .iph.nexthdr = 6, > >> - .iph.payload_len = bpf_htons(MAGIC_BYTES), > >> + .iph.payload_len = __constant_htons(MAGIC_BYTES), > >> .tcp.urg_ptr = 123, > >> }; > >> > >> -- > >> 2.20.0.rc2.403.gdbc3b29805-goog > >> >
On 12/12/2018 04:13 AM, Stanislav Fomichev wrote: > On 12/12, Daniel Borkmann wrote: >> On 12/11/2018 10:49 PM, Stanislav Fomichev wrote: >>> On 12/11, Stanislav Fomichev wrote: >>>> We're using bpf_htons in test_progs.c to initialize some static >>>> global data and I think I hit some weird case on an older compiler >>>> which doesn't have __builtin_bswap16 (and __builtin_constant_p >>>> expands to false). >>>> >>>> In this case I see: >>>> error: implicit declaration of function '__builtin_bswap16' >> >> Is that gcc < 4.8? > Yes. > >>>> Let's explicitly use __constant_htons which should be exposed by the >>>> linux/byteorder.h uapi header. >>> >>> Forgot to mention, that using simple htons produces the following: >>> test_progs.c:54:17: error: braced-group within expression allowed only >>> inside a function >>> .eth.h_proto = htons(ETH_P_IP), >>> >>>> Signed-off-by: Stanislav Fomichev <sdf@google.com> >>>> --- >>>> tools/testing/selftests/bpf/test_progs.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c >>>> index 26f1fdf3e2bf..61593d319c0e 100644 >>>> --- a/tools/testing/selftests/bpf/test_progs.c >>>> +++ b/tools/testing/selftests/bpf/test_progs.c >>>> @@ -51,10 +51,10 @@ static struct { >>>> struct iphdr iph; >>>> struct tcphdr tcp; >>>> } __packed pkt_v4 = { >>>> - .eth.h_proto = bpf_htons(ETH_P_IP), >>>> + .eth.h_proto = __constant_htons(ETH_P_IP), >> >> If the __builtin_constant_p() evaluated to false on the constants (?), >> wouldn't using the __bpf_constant_htons() directly work as well given >> it's not using a builtin either? Should be fine either way though using >> the same api/header might be slightly nicer. > I got it wrong, __builtin_constant_p() evaluates correctly, I played > with it a bit. But for some reason it still complains about that branch > that it doesn't take :-/ > > Using __bpf_constant_htons() is a good idea, I'll follow up with a v2. Ok, sounds good, thanks!
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index 26f1fdf3e2bf..61593d319c0e 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -51,10 +51,10 @@ static struct { struct iphdr iph; struct tcphdr tcp; } __packed pkt_v4 = { - .eth.h_proto = bpf_htons(ETH_P_IP), + .eth.h_proto = __constant_htons(ETH_P_IP), .iph.ihl = 5, .iph.protocol = 6, - .iph.tot_len = bpf_htons(MAGIC_BYTES), + .iph.tot_len = __constant_htons(MAGIC_BYTES), .tcp.urg_ptr = 123, }; @@ -64,9 +64,9 @@ static struct { struct ipv6hdr iph; struct tcphdr tcp; } __packed pkt_v6 = { - .eth.h_proto = bpf_htons(ETH_P_IPV6), + .eth.h_proto = __constant_htons(ETH_P_IPV6), .iph.nexthdr = 6, - .iph.payload_len = bpf_htons(MAGIC_BYTES), + .iph.payload_len = __constant_htons(MAGIC_BYTES), .tcp.urg_ptr = 123, };
We're using bpf_htons in test_progs.c to initialize some static global data and I think I hit some weird case on an older compiler which doesn't have __builtin_bswap16 (and __builtin_constant_p expands to false). In this case I see: error: implicit declaration of function '__builtin_bswap16' Let's explicitly use __constant_htons which should be exposed by the linux/byteorder.h uapi header. Signed-off-by: Stanislav Fomichev <sdf@google.com> --- tools/testing/selftests/bpf/test_progs.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)