Skip to content

Commit 77f769e

Browse files
Jordan Romejordalgo
Jordan Rome
authored and
jordalgo
committed
Remove multi-map delete API
This is no longer valid: ``` @A = 1; @b[1] = 2; delete(@A, @b[1]); ``` This also fixes map key resizing for the `delete` function: this is no longer an error: `@y["hi"] = 5; delete(@y, "longerstr");` #3495 #3496
1 parent 84cfa95 commit 77f769e

File tree

7 files changed

+141
-72
lines changed

7 files changed

+141
-72
lines changed

‎CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@ and this project adheres to
1212
- Return `uint32` instead of `uint64` for `pid` and `tid` builtins
1313
- [#3441](https://github.com/bpftrace/bpftrace/pull/3441)
1414
- [Migration guide](docs/migration_guide.md#pid-and-tid-builtins-return-uint32)
15+
- Remove multi-map `delete` functionality
16+
- [#3506](https://github.com/bpftrace/bpftrace/pull/3506)
17+
- [Migration guide](docs/migration_guide.md#multi-key-delete-removed)
1518
#### Added
1619
- Add `--dry-run` CLI option
1720
- [#3203](https://github.com/bpftrace/bpftrace/pull/3203)

‎docs/migration_guide.md

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,46 @@ compatibility in some way. Each entry should contain:
99

1010
## Versions 0.21.x (or earlier) to 0.22.x (or later)
1111

12+
### multi-key `delete` removed
13+
14+
https://github.com/bpftrace/bpftrace/pull/3506
15+
16+
This map `delete` syntax is no longer valid:
17+
```
18+
delete(@b[1], @b[2], @b[3]);
19+
```
20+
And will yield this error:
21+
```
22+
# bpftrace -e 'BEGIN { @b[1] = 1; delete(@b[1], @b[2], @b[3]); }'
23+
stdin:1:20-47: ERROR: delete() takes up to 2 arguments (3 provided)
24+
BEGIN { @b[1] = 1; delete(@b[1], @b[2], @b[3]); }
25+
~~~~~~~~~~~~~~~~~~~~~~~~~~~
26+
```
27+
28+
You might also see this error:
29+
```
30+
# bpftrace -e 'BEGIN { @b[1] = 1; delete(@b[1], @b[2]); }'
31+
stdin:1:20-32: ERROR: delete() expects a map with no keys for the first argument
32+
BEGIN { @b[1] = 1; delete(@b[1], @b[2]); }
33+
~~~~~~~~~~~~
34+
```
35+
36+
`delete` now expects only two arguments: a map and a key. For example, the above
37+
delete statement should be rewritten as this:
38+
```
39+
delete(@b, 1);
40+
delete(@b, 2);
41+
delete(@b, 3);
42+
```
43+
44+
And for maps with multiple values as keys, which are represented as a tuple,
45+
the delete call looks like this:
46+
```
47+
@c[1, "hello"] = 1;
48+
49+
delete(@c, (1, "hello"));
50+
```
51+
1252
### `pid` and `tid` builtins return `uint32`
1353

1454
https://github.com/bpftrace/bpftrace/pull/3441

‎man/adoc/bpftrace.adoc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2505,7 +2505,7 @@ interval:s:10 {
25052505

25062506
.variants
25072507
* `delete(map m, mapkey k)`
2508-
* deprecated `delete(mapkey k, ...)`
2508+
* deprecated `delete(mapkey k)`
25092509

25102510
Delete a single key from a map.
25112511
For scalar maps (e.g. no explicit keys), the key is omitted and is equivalent to calling `clear`.

‎src/ast/passes/codegen_llvm.cpp

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -756,18 +756,25 @@ void CodegenLLVM::visit(Call &call)
756756
b_.CreateLifetimeEnd(key);
757757
expr_ = nullptr;
758758
} else if (call.func == "delete") {
759-
for (const auto &arg : call.vargs) {
760-
auto &map = static_cast<Map &>(*arg);
761-
auto [key, scoped_key_deleter] = getMapKey(map);
762-
if (!is_bpf_map_clearable(map_types_[map.ident])) {
763-
// store zero instead of calling bpf_map_delete_elem()
764-
AllocaInst *val = b_.CreateAllocaBPF(map.type, map.ident + "_zero");
765-
b_.CreateStore(Constant::getNullValue(b_.GetType(map.type)), val);
766-
b_.CreateMapUpdateElem(ctx_, map.ident, key, val, call.loc);
767-
b_.CreateLifetimeEnd(val);
768-
} else {
769-
b_.CreateMapDeleteElem(ctx_, map, key, call.loc);
770-
}
759+
auto &arg0 = *call.vargs.at(0);
760+
auto &map = static_cast<Map &>(arg0);
761+
// Current API: delete accepts two arguments except in the case of scalar
762+
// maps (maps with no keys) in which case it you can just pass it the map
763+
// and it will act similar to `clear` e.g. `delete(@scalar);`
764+
// Legacy API: delete accepts a single argument that is the map with a
765+
// key expression e.g. `delete(@mymap[1, 2]);` or no key if the map
766+
// is a scalar
767+
auto [key, scoped_key_deleter] = call.vargs.size() > 1
768+
? getMapKey(map, call.vargs.at(1))
769+
: getMapKey(map);
770+
if (!is_bpf_map_clearable(map_types_[map.ident])) {
771+
// store zero instead of calling bpf_map_delete_elem()
772+
AllocaInst *val = b_.CreateAllocaBPF(map.type, map.ident + "_zero");
773+
b_.CreateStore(Constant::getNullValue(b_.GetType(map.type)), val);
774+
b_.CreateMapUpdateElem(ctx_, map.ident, key, val, call.loc);
775+
b_.CreateLifetimeEnd(val);
776+
} else {
777+
b_.CreateMapDeleteElem(ctx_, map, key, call.loc);
771778
}
772779
expr_ = nullptr;
773780
} else if (call.func == "has_key") {

‎src/ast/passes/semantic_analyser.cpp

Lines changed: 33 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -588,9 +588,16 @@ void SemanticAnalyser::visit(Call &call)
588588

589589
if (expr.is_map) {
590590
Map &map = static_cast<Map &>(expr);
591+
591592
// If the map is indexed, don't skip key validation
592-
if (map.key_expr == nullptr && skip_key_validation(call))
593-
map.skip_key_validation = true;
593+
if (map.key_expr == nullptr) {
594+
// Delete expects just a map reference for the first argument
595+
if (call.func == "delete" && i == 0) {
596+
map.skip_key_validation = true;
597+
} else if (skip_key_validation(call)) {
598+
map.skip_key_validation = true;
599+
}
600+
}
594601
}
595602

596603
Visit(call.vargs[i]);
@@ -724,46 +731,34 @@ void SemanticAnalyser::visit(Call &call)
724731
call.type = CreateStats(true);
725732
} else if (call.func == "delete") {
726733
check_assignment(call, false, false, false);
727-
if (check_varargs(call, 1, std::numeric_limits<size_t>::max())) {
728-
if (call.vargs.size() == 2) {
729-
if (!call.vargs.at(0)->is_map)
730-
LOG(ERROR, call.vargs.at(0)->loc, err_) << DELETE_ERROR;
731-
732-
auto *key_arg = call.vargs.at(1);
733-
if (!key_arg->is_map) {
734-
Map &map = static_cast<Map &>(*call.vargs.at(0));
734+
if (check_varargs(call, 1, 2)) {
735+
if (!call.vargs.at(0)->is_map) {
736+
LOG(ERROR, call.vargs.at(0)->loc, err_) << DELETE_ERROR;
737+
} else {
738+
Map &map = static_cast<Map &>(*call.vargs.at(0));
739+
if (call.vargs.size() == 1) {
740+
if (map.key_expr) {
741+
// We're modifying the AST here to support the deprecated delete
742+
// API. Once we remove the old API, we can delete this.
743+
call.vargs.push_back(map.key_expr);
744+
map.key_expr = nullptr;
745+
} else if (is_final_pass()) {
746+
auto *map_key_type = get_map_key_type(map);
747+
if (map_key_type && !map_key_type->IsNoneTy()) {
748+
LOG(ERROR, call.vargs.at(0)->loc, err_) << DELETE_ERROR;
749+
}
750+
}
751+
} else {
735752
if (map.key_expr) {
736753
LOG(ERROR, call.vargs.at(0)->loc, err_)
737754
<< "delete() expects a map with no keys for the first argument";
738-
} else {
739-
auto *mapkey = get_map_key_type(map);
740-
if (mapkey) {
741-
auto key_type = create_key_type(call.vargs.at(1)->type,
742-
call.vargs.at(1)->loc);
743-
if (!mapkey->IsSameType(key_type) ||
744-
!key_type.FitsInto(*mapkey)) {
745-
LOG(ERROR, call.vargs.at(1)->loc, err_)
746-
<< "Argument mismatch for " << map.ident << ": "
747-
<< "trying to delete with key of type: '"
748-
<< call.vargs.at(1)->type << "' when map has key of type: '"
749-
<< *mapkey << "'";
750-
}
751-
}
752755
}
753-
754-
// We're modifying the AST here to support the deprecated delete
755-
// API so subsequent passes will fall through to the else statement
756-
// below. Once we remove the old API, we can handle this properly.
757-
map.key_expr = call.vargs.at(1);
758-
call.vargs.pop_back();
759-
}
760-
} else {
761-
// This supports the deprecated delete API of passing multiple maps as
762-
// args
763-
for (const auto *arg : call.vargs) {
764-
if (!arg->is_map) {
765-
LOG(ERROR, arg->loc, err_) << DELETE_ERROR;
766-
break;
756+
auto *map_key_type = get_map_key_type(map);
757+
if (map_key_type) {
758+
auto &arg1 = *call.vargs.at(1);
759+
SizedType new_key_type = create_key_type(arg1.type, arg1.loc);
760+
update_current_key(*map_key_type, new_key_type);
761+
validate_new_key(*map_key_type, new_key_type, map.ident, arg1.loc);
767762
}
768763
}
769764
}

‎tests/runtime/basic

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -233,10 +233,10 @@ PROG BEGIN { @ = count(); @a[1] = count(); delete(@); delete(@a, 1); exit(); }
233233
EXPECT @: 0
234234
TIMEOUT 1
235235

236-
NAME delete multiple-map deprecated
237-
PROG BEGIN { @ = 1; @a[1] = 1; delete(@, @a[1]); exit(); }
238-
EXPECT_NONE @: 1
236+
NAME delete deprecated
237+
PROG BEGIN { @a[1] = 1; @b[2, "hi"] = 2; delete(@a[1]); delete(@b[2, "hi"]); exit(); }
239238
EXPECT_NONE @a[1]: 1
239+
EXPECT_NONE @b[2, hi]: 2
240240
TIMEOUT 1
241241

242242
NAME increment/decrement map

‎tests/semantic_analyser.cpp

Lines changed: 42 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -865,8 +865,13 @@ TEST(semantic_analyser, call_delete)
865865
{
866866
test("kprobe:f { @x = 1; delete(@x); }");
867867
test("kprobe:f { @y[5] = 5; delete(@y, 5); }");
868+
test("kprobe:f { @a[1] = 1; delete(@a, @a[1]); }");
869+
test("kprobe:f { @a = 1; @b[2] = 2; delete(@b, @a); }");
870+
test("kprobe:f { @a[1] = 1; $x = 1; delete(@a, $x); }");
871+
test(R"(kprobe:f { @y["hi"] = 5; delete(@y, "longerstr"); })");
868872
test(R"(kprobe:f { @y["hi", 5] = 5; delete(@y, ("hi", 5)); })");
869873
test(R"(kprobe:f { @y["longerstr", 5] = 5; delete(@y, ("hi", 5)); })");
874+
test(R"(kprobe:f { @y["hi", 5] = 5; delete(@y, ("longerstr", 5)); })");
870875
test("kprobe:f { @y[(3, 4, 5)] = 5; delete(@y, (1, 2, 3)); }");
871876
test("kprobe:f { @y[((int8)3, 4, 5)] = 5; delete(@y, (1, 2, 3)); }");
872877
test("kprobe:f { @y[(3, 4, 5)] = 5; delete(@y, ((int8)1, 2, 3)); }");
@@ -917,32 +922,51 @@ kprobe:f { @y[5] = 5; delete(@y[5], 5); }
917922
)");
918923

919924
test_error("kprobe:f { @y[(3, 4, 5)] = 5; delete(@y, (1, 2)); }", R"(
920-
stdin:1:42-48: ERROR: Argument mismatch for @y: trying to delete with key of type: '(int64,int64)' when map has key of type: '(int64,int64,int64)'
925+
stdin:1:42-48: ERROR: Argument mismatch for @y: trying to access with arguments: '(int64,int64)' when map expects arguments: '(int64,int64,int64)'
921926
kprobe:f { @y[(3, 4, 5)] = 5; delete(@y, (1, 2)); }
922927
~~~~~~
923928
)");
924929

925-
test_error(R"(kprobe:f { @y["hi", 5] = 5; delete(@y, ("hiandbye", 5)); })",
926-
R"(
927-
stdin:1:40-55: ERROR: Argument mismatch for @y: trying to delete with key of type: '(string[9],int64)' when map has key of type: '(string[3],int64)'
928-
kprobe:f { @y["hi", 5] = 5; delete(@y, ("hiandbye", 5)); }
929-
~~~~~~~~~~~~~~~
930+
test_error("kprobe:f { @y[1] = 2; delete(@y); }", R"(
931+
stdin:1:23-32: ERROR: delete() expects a map for the first argument and a key for the second argument e.g. `delete(@my_map, 1);`
932+
kprobe:f { @y[1] = 2; delete(@y); }
933+
~~~~~~~~~
934+
)");
935+
936+
test_error("kprobe:f { @a[1] = 1; delete(@a, @a); }", R"(
937+
stdin:1:34-36: ERROR: Argument mismatch for @a: trying to access with no arguments when map expects arguments: 'int64'
938+
kprobe:f { @a[1] = 1; delete(@a, @a); }
939+
~~
930940
)");
931941

932942
// Deprecated API
933943
test("kprobe:f { @x = 1; delete(@x); }");
934-
test("kprobe:f { @x = 1; @y = 2; delete(@x, @y); }");
935-
test("kprobe:f { @x = 1; @y[5] = 5; delete(@x, @y[5]); }");
936-
937-
test_error("kprobe:f { @x = 1; @y[5] = 5; delete(@x, @y); }", R"(
938-
stdin:1:42-44: ERROR: Argument mismatch for @y: trying to access with no arguments when map expects arguments: 'int64'
939-
kprobe:f { @x = 1; @y[5] = 5; delete(@x, @y); }
940-
~~
941-
)");
942-
test_error("kprobe:f { @x = 1; $y = 2; $c = 3; delete(@x, $y, $c); }", R"(
943-
stdin:1:47-49: ERROR: delete() expects a map for the first argument and a key for the second argument e.g. `delete(@my_map, 1);`
944-
kprobe:f { @x = 1; $y = 2; $c = 3; delete(@x, $y, $c); }
945-
~~
944+
test("kprobe:f { @y[5] = 5; delete(@y[5]); }");
945+
test(R"(kprobe:f { @y[1, "hi"] = 5; delete(@y[1, "longerstr"]); })");
946+
test(R"(kprobe:f { @y[1, "longerstr"] = 5; delete(@y[1, "hi"]); })");
947+
948+
test_error("kprobe:f { @x = 1; @y = 5; delete(@x, @y); }", R"(
949+
stdin:1:39-41: ERROR: Argument mismatch for @x: trying to access with arguments: 'int64' when map expects no arguments
950+
kprobe:f { @x = 1; @y = 5; delete(@x, @y); }
951+
~~
952+
)");
953+
954+
test_error(R"(kprobe:f { @x[1, "hi"] = 1; delete(@x["hi", 1]); })", R"(
955+
stdin:1:29-47: ERROR: Argument mismatch for @x: trying to access with arguments: '(string[3],int64)' when map expects arguments: '(int64,string[3])'
956+
kprobe:f { @x[1, "hi"] = 1; delete(@x["hi", 1]); }
957+
~~~~~~~~~~~~~~~~~~
958+
)");
959+
960+
test_error("kprobe:f { @x = 1; @y[5] = 5; delete(@x, @y[5], @y[6]); }", R"(
961+
stdin:1:31-55: ERROR: delete() takes up to 2 arguments (3 provided)
962+
kprobe:f { @x = 1; @y[5] = 5; delete(@x, @y[5], @y[6]); }
963+
~~~~~~~~~~~~~~~~~~~~~~~~
964+
)");
965+
966+
test_error("kprobe:f { @x = 1; delete(@x[1]); }", R"(
967+
stdin:1:20-31: ERROR: Argument mismatch for @x: trying to access with arguments: 'int64' when map expects no arguments
968+
kprobe:f { @x = 1; delete(@x[1]); }
969+
~~~~~~~~~~~
946970
)");
947971
}
948972

0 commit comments

Comments
 (0)