On 5/17/23 2:01 AM, Lorenz Bauer wrote:
On Wed, May 17, 2023 at 7:26 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote:
On 5/15/23 5:15 AM, Lorenz Bauer wrote:
In commit 9b459804ff99 ("btf: fix resolving BTF_KIND_VAR after ARRAY, STRUCT, UNION, PTR")
I fixed a bug that occurred during resolving of a DATASEC by strategically resetting
resolve_mode. This fixes the immediate bug but leaves us open to future bugs where
nested types have to be resolved.
hmm... future bugs like when adding new BTF_KIND in the future?
It could just be refactoring of the codebase? What is the downside of
restoring the mode when popping the item? It also makes push and pop
symmetrical.
I can see your point to refactor it to make it work for all different BTF_KIND.
Other than BTF_KIND_DATASEC, env->resolve_mode stays the same for all other
kinds once it is decided. It is why resolve_mode is in the "env" instead of "v".
My concern is this will hide some bugs (existing or future) that accidentally
changed the resolve_mode in the middle. If there is another legit case that
could be found other than BTF_KIND_DATASEC, that will be a better time to do
this refactoring with a proper test case considering most bpf progs need btf to
load nowadays and probably need to veristat test also. If it came to that, might
as well consider moving resolve_mode from "env" to "v".
btf_datasec_resolve() is acting as a very top level resolver like btf_resolve(),
so it reset env->resolve_mode before resolving its var member like how
btf_resolve() does. imo, together with env->resolve_mode stays the same for
others, it is more straight forward to reason. I understand that it is personal
preference and could argue either way.
