[bpf-next] bpf: Fix a 'unused function' compilation error

Commit Message

Yonghong Song Jan. 12, 2024, 4:16 a.m. UTC

Building the kernel with latest llvm18, I hit the following error:

 /home/yhs/work/bpf-next/kernel/bpf/verifier.c:4383:13: error: unused function '__is_scalar_unbounded' [-Werror,-Wunused-function]
  4383 | static bool __is_scalar_unbounded(struct bpf_reg_state *reg)
       |             ^~~~~~~~~~~~~~~~~~~~~
 1 error generated.

Patches [1] and [2] are in the same patch set. Patch [1] removed
the usage of __is_scalar_unbounded(), and patch [2] re-introduced
the usage of the function. Currently patch [1] is merged into
bpf-next while patch [2] does not, hence the above compilation
error is triggered.

To fix the compilation failure, let us temporarily make
__is_scalar_unbounded() not accessible through macro '#if 0'.
It can be re-introduced later when [2] is ready to merge.

  [1] https://lore.kernel.org/bpf/[e-mail]/
  [2] https://lore.kernel.org/bpf/[e-mail]/

Signed-off-by: Yonghong Song <[e-mail]>
---
 kernel/bpf/verifier.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Alexei Starovoitov Jan. 12, 2024, 4:59 a.m. UTC | #1

On Thu, Jan 11, 2024 at 8:17 PM Yonghong Song <[e-mail]> wrote:
>
> Building the kernel with latest llvm18, I hit the following error:
>
>  /home/yhs/work/bpf-next/kernel/bpf/verifier.c:4383:13: error: unused function '__is_scalar_unbounded' [-Werror,-Wunused-function]
>   4383 | static bool __is_scalar_unbounded(struct bpf_reg_state *reg)
>        |             ^~~~~~~~~~~~~~~~~~~~~
>  1 error generated.
>
> Patches [1] and [2] are in the same patch set. Patch [1] removed
> the usage of __is_scalar_unbounded(), and patch [2] re-introduced
> the usage of the function. Currently patch [1] is merged into
> bpf-next while patch [2] does not, hence the above compilation
> error is triggered.
>
> To fix the compilation failure, let us temporarily make
> __is_scalar_unbounded() not accessible through macro '#if 0'.
> It can be re-introduced later when [2] is ready to merge.
>
>   [1] https://lore.kernel.org/bpf/[e-mail]/
>   [2] https://lore.kernel.org/bpf/[e-mail]/

Ouch. Sorry. This interaction between patches was unexpected.
Instead of this particular if 0 patch, is there a way to amend pushed
patches to avoid this issue?

Yonghong Song Jan. 12, 2024, 5:29 a.m. UTC | #2

On 1/11/24 8:59 PM, Alexei Starovoitov wrote:
> On Thu, Jan 11, 2024 at 8:17 PM Yonghong Song <[e-mail]> wrote:
>> Building the kernel with latest llvm18, I hit the following error:
>>
>>   /home/yhs/work/bpf-next/kernel/bpf/verifier.c:4383:13: error: unused function '__is_scalar_unbounded' [-Werror,-Wunused-function]
>>    4383 | static bool __is_scalar_unbounded(struct bpf_reg_state *reg)
>>         |             ^~~~~~~~~~~~~~~~~~~~~
>>   1 error generated.
>>
>> Patches [1] and [2] are in the same patch set. Patch [1] removed
>> the usage of __is_scalar_unbounded(), and patch [2] re-introduced
>> the usage of the function. Currently patch [1] is merged into
>> bpf-next while patch [2] does not, hence the above compilation
>> error is triggered.
>>
>> To fix the compilation failure, let us temporarily make
>> __is_scalar_unbounded() not accessible through macro '#if 0'.
>> It can be re-introduced later when [2] is ready to merge.
>>
>>    [1] https://lore.kernel.org/bpf/[e-mail]/
>>    [2] https://lore.kernel.org/bpf/[e-mail]/
> Ouch. Sorry. This interaction between patches was unexpected.
> Instead of this particular if 0 patch, is there a way to amend pushed
> patches to avoid this issue?

Another option is that in merged patch [1] removing the function __is_scalar_unbounded().
And the function can be re-introduced later if needed.

Alexei Starovoitov Jan. 12, 2024, 6:02 a.m. UTC | #3

On Thu, Jan 11, 2024 at 9:29 PM Yonghong Song <[e-mail]> wrote:
>
>
> On 1/11/24 8:59 PM, Alexei Starovoitov wrote:
> > On Thu, Jan 11, 2024 at 8:17 PM Yonghong Song <[e-mail]> wrote:
> >> Building the kernel with latest llvm18, I hit the following error:
> >>
> >>   /home/yhs/work/bpf-next/kernel/bpf/verifier.c:4383:13: error: unused function '__is_scalar_unbounded' [-Werror,-Wunused-function]
> >>    4383 | static bool __is_scalar_unbounded(struct bpf_reg_state *reg)
> >>         |             ^~~~~~~~~~~~~~~~~~~~~
> >>   1 error generated.
> >>
> >> Patches [1] and [2] are in the same patch set. Patch [1] removed
> >> the usage of __is_scalar_unbounded(), and patch [2] re-introduced
> >> the usage of the function. Currently patch [1] is merged into
> >> bpf-next while patch [2] does not, hence the above compilation
> >> error is triggered.
> >>
> >> To fix the compilation failure, let us temporarily make
> >> __is_scalar_unbounded() not accessible through macro '#if 0'.
> >> It can be re-introduced later when [2] is ready to merge.
> >>
> >>    [1] https://lore.kernel.org/bpf/[e-mail]/
> >>    [2] https://lore.kernel.org/bpf/[e-mail]/
> > Ouch. Sorry. This interaction between patches was unexpected.
> > Instead of this particular if 0 patch, is there a way to amend pushed
> > patches to avoid this issue?
>
> Another option is that in merged patch [1] removing the function __is_scalar_unbounded().
> And the function can be re-introduced later if needed.

Right. That will work, but it's not a tiny function.
So it's a code churn to remove it in a commit just to add it back
a few commits later.

To fix the build I dropped the last two commits,
but the issue remains.
If they're resend in the same shape later they will
re-introduce a bisect issue. Hence the question,
how we can tweak the patch "bpf: Track spilled unbounded scalars"
so it doesn't leave behind an unused static function.

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 7ddad07ae928..e1f42082f32f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4380,6 +4380,7 @@  static u64 reg_const_value(struct bpf_reg_state *reg, bool subreg32)
 	return subreg32 ? tnum_subreg(reg->var_off).value : reg->var_off.value;
 }
 
+#if 0
 static bool __is_scalar_unbounded(struct bpf_reg_state *reg)
 {
 	return tnum_is_unknown(reg->var_off) &&
@@ -4388,6 +4389,7 @@  static bool __is_scalar_unbounded(struct bpf_reg_state *reg)
 	       reg->s32_min_value == S32_MIN && reg->s32_max_value == S32_MAX &&
 	       reg->u32_min_value == 0 && reg->u32_max_value == U32_MAX;
 }
+#endif
 
 static bool __is_pointer_value(bool allow_ptr_leaks,
 			       const struct bpf_reg_state *reg)