Type comparisons can occur before type propagation

[ajor]

This script should be valid:

# bpftrace -e 'BEGIN { @y = 0; @y = @x; @x = 1; }'
stdin:1:17-19: ERROR: Type mismatch for @y: trying to assign value of type 'none' when map already contains a value of type 'int64
BEGIN { @y = 0; @y = @x; @x = 1; }
                ~~

Related to #2979

[fbs]

Do you mean that @x should get its type from @x =1?

Imo this should be an 'use of undeclared identifier @x' or something similar error, as x isn't properly defined at this point

[ajor]

Do you mean that @x should get its type from @x =1?

Yeah. Maps aren't required to be defined before they're used, since they're shared across probes and so the order that they appear in a bpftrace script isn't necessarily the order in which they get read/updated at runtime.

As another example, this script is valid:

BEGIN { @x = 1; }
tracepoint:syscalls:sys_enter_read { @y = 0; @y = @x }

But this script currently errors with the same message as the original issue:

tracepoint:syscalls:sys_enter_read { @y = 0; @y = @x }
BEGIN { @x = 1; }

This second script should still work as it's semantically identical to the first. The order in which probes are defined (and therefore maps are defined too) shouldn't change things.

[fbs]

Ah I see your reasoning although I like the 2nd example more than the first.

E.g. in BEGIN { @y = 1; @y = @x; @x = 2; }. What should the value of y be? Is it 1, 2 or 3 (or invalid)? Seems like fixing this can mask bugs in more complex programs, accidentally assigning a value from an 'empty' map.

For two+ probes it would indeed be nice if it detects the type independent of probe order. But maybe its better to just have the option to define maps up front?

[ajor]

Agreed that declaring maps up front would be useful for big programs (opened an issue for it here: #2954). But type deduction is useful for ad-hoc investigations and we shouldn't give that up.

Maps have always been initialised with a default value: 0 for integers, "" for strings, etc. (I'm not sure what for stacks...). So in your example, @y would finish at 0.

There could be an argument for changing this behaviour, but the initial example in this issue is specifically a bug with the current implementation. If we remove the @y = 0 at the beginning then the assignment-before definition works as designed:

# bpftrace -e 'BEGIN { @y = @x; @x = 1; }'
Attaching 1 probe...
^C

@x: 1
@y: 0

[ajor]

I am thinking that it might be useful to have a "strict mode" which prints errors at runtime if a program attempts to read from an undefined map (it can't be detected at compile time when there are multiple probes involved).

[danobi]

I am thinking that it might be useful to have a "strict mode" which prints errors at runtime if a program attempts to read from an undefined map (it can't be detected at compile time when there are multiple probes involved).

Seems like -kk should be catching that case:

bpftrace/src/ast/irbuilderbpf.cpp

Line 430 in 72d54f2

CreateHelperError(ctx, getInt32(0), libbpf::BPF_FUNC_map_lookup_elem, loc);

But doesn't seem to work:

$ sudo ./build/src/bpftrace -e 'BEGIN { print(@) } END { @ = 1 }' -kk
Attaching 2 probes...

^C

@: 1

[danobi]

Upfront map definitions sounds useful. But perhaps would be good to think about if we can soundly determine map types in all cases. If it's not possible, I think it's reasonable to say something like: "bpftrace does its best to infer map types but in pathological cases it's better for you to annotate the map types".

[jordalgo]

Maybe a tangent but just curious: what should the output of this be?

$ sudo ./build/src/bpftrace -e 'BEGIN { print(@) } END { @ = 1 }'

Should it print the map twice? Once with @: 0 and again at the end when it dumps the map @: 1 ?

[ajor]

Should it print the map twice? Once with @: 0 and again at the end when it dumps the map @: 1 ?

Yep, exactly. The print will read the default 0 value from the uninitialised map.

EDIT: Ah... but it does not! print(@) currently prints an empty string in this case. printf("%d\n", @) prints 0 as expected.

[jordalgo]

Issue filed for the empty map printing: #3006

[ajor]

I am thinking that it might be useful to have a "strict mode" which prints errors at runtime if a program attempts to read from an undefined map (it can't be detected at compile time when there are multiple probes involved).

Seems like -kk should be catching that case:

bpftrace/src/ast/irbuilderbpf.cpp

Line 430 in 72d54f2

CreateHelperError(ctx, getInt32(0), libbpf::BPF_FUNC_map_lookup_elem, loc);

But doesn't seem to work:

$ sudo ./build/src/bpftrace -e 'BEGIN { print(@) } END { @ = 1 }' -kk
Attaching 2 probes...

^C

@: 1

It looks like -kk does work as you described for printf:

$ sudo bpftrace -e 'BEGIN { printf("%d\n", @x) } END { @x = 1; }' -kk
Attaching 2 probes...
stdin:1:24-26: WARNING: Can't lookup map element because it does not exist.
Additional Info - helper: map_lookup_elem, retcode: 0
BEGIN { printf("%d\n", @x) } END { @x = 1; }
                       ~~
0

I think it not working with print must be because that printing happens asynchronously in userspace. It would be good to change that for other reasons as well: #3028.

[lenticularis39]

The same issue also happens for the function call implementation in #3068, since the type of the function is not resolved yet in the first pass:

# bpftrace -e 'fn f(): int64 { print("Hello"); } BEGIN { $x = 1; $x = f(); }'
stdin:1:52-60: ERROR: Type mismatch for $x: trying to assign value of type 'none' when variable already contains a value of type 'int64'
fn f(): int64 { print("Hello"); } BEGIN { $x = 1; $x = f(); }

I fixed by adding is_final_pass() to the type check, which also solves the problem reported here. In the future, this will be solved by #2979.