Array improvements #1656

viktormalik · 2020-12-04T09:32:23Z

This improves work with constant arrays. Until now, user could only use [] to directly access particular array elements.
This PR adds new possibilities to:

assign an array to a local variable (resolves Can't assign Type::Array value to a local variable #1057)
assign an array to a map
use an array as a map key (resolves Support Type::Array as a map key #1052).

If an array is assigned into a variable, operator [] can be later used on that variable to access the array elements.

For local variables, only the pointer to the array is stored. For maps, the array is fully copied into the map (or into the map key) since it may be accessed from another probe where the original array may not exist anymore, or may have a different value. I'm not sure if this is a correct approach, feel free to suggest something better.

The last commit also slightly changes the type of alloca for map key - instead of always allocating an array of bytes, it allocates the appropriate type (e.g. i64 instead of [8 * i8] for integer keys). This caused some changes in existing codegens.

Checklist

Language changes are updated in docs/reference_guide.md
User-visible and non-trivial changes updated in CHANGELOG.md
The new behaviour is covered by tests

viktormalik · 2020-12-04T09:38:39Z

CI failed on fetching packages, needs restart

src/ast/codegen_llvm.cpp

danobi · 2021-01-12T02:36:45Z

src/ast/codegen_llvm.cpp

@@ -2275,10 +2275,32 @@ AllocaInst *CodegenLLVM::getMapKey(Map &map)
      }
      else


So with this code, you can have arrays as the only key to a map entry. What about support for 2 or more values as a map key?

Good point, I'll add support for array used as a part of multi-value key, it should be quite straightforward.

danobi · 2021-01-12T02:38:38Z

Can you also add some runtime tests for more complex array types? eg an array of structs and nested arrays.

viktormalik · 2021-01-14T16:43:13Z

Can you also add some runtime tests for more complex array types? eg an array of structs and nested arrays.

Done. It required some small changes, so there's one more commit to review.

I also added support for array as a part of multi-key.

mmisono · 2021-01-20T05:37:23Z

Nice changes, but it seems there is a problem with handling an array in a tuple.

% sudo ./src/bpftrace -e 'BEGIN {@a = curtask->numa_faults_locality; @b = (1, curtask->numa_faults_locali
ty);}'
Attaching 1 probe...
^C

@a: [0,7,0]

@b: (1, [18446619453034875792,0,0])

viktormalik · 2021-01-20T09:38:59Z

Nice changes, but it seems there is a problem with handling an array in a tuple.

Right, good observation. In fact, neither structs can be used in tuples. Also, a good implementation wouldn't be exactly simple, so I'd prefer disabling this (array or struct in a tuple) for now and implementing it later.

danobi · 2021-01-20T23:08:26Z

src/ast/codegen_llvm.cpp

+    {
+      val = b_.CreatePtrToInt(expr_, b_.getInt64Ty());
+      // Since only the pointer is copied, we need to extend lifetime of RHS
+      scoped_del.disarm();


Are you sure this is what you want? Lifetime extension is usually done with:

expr_deleter_ = scoped_del.disarm();

That will only extend lifetime to the end of the current statement but I need to extend it to the end of the probe (since other statements may want to read from that pointer). I haven't found any other way to do that, so this seemed like the simplest solution.

Anyway, I plan to improve work with structs and arrays (to avoid unnecessary memcpys), so I'll probably come with a cleaner solution, then.

danobi · 2021-01-20T23:27:34Z

Right, good observation. In fact, neither structs can be used in tuples. Also, a good implementation wouldn't be exactly simple, so I'd prefer disabling this (array or struct in a tuple) for now and implementing it later.

Sure, sounds reasonable. Can you add a check to semantic analyser, then?

Since arrays are treated as constant, it is sufficient to assign the pointer to the beginning of the array into the variable. This allows to use the element access (operator []) later on the local variable. Small refactoring of the array runtime tests (shorter naming of the struct).

When assigning into a map, the array is copied (similarly to struct). It can be later retrieved from the map and individual elements can be accessed (using operator []). Current requirement is that only arrays of the same type (including the size) can be stored inside a map. This commit also improves work with array types. IsArray is renamed to IsByteArray and it now covers strings and other types allocated per-bytes. Arrays are allocated as closely to their actual type as possible. Computation of the size and the number of elements of an array is fixed.

If an array is used as a map key, it is entirely copied into the key. The key may be composed of multiple values. This commit slightly changes the size of alloca used for a map key - instead of always allocating array of bytes, the appropriate type is now allocated (e.g. i64 instead of [8 * i8]). This caused some changes in existing codegens.

On array element access, do not load from the element pointer if the element is not of a scalar type. Instead, pass the pointer further. There are some changes in struct_array runtime tests: - one array was made smaller as it caused BPF stack overflow when it was read into a map (this could be probably fixed by removing some unnecessary alloca and memcpy instructions) - one scalar was changed to an array to create array of struct containing another array

danobi added the waiting-for-feedback label Jan 5, 2021

danobi reviewed Jan 12, 2021

View reviewed changes

danobi removed the waiting-for-feedback label Jan 13, 2021

viktormalik force-pushed the arrays-improvements branch from bf81f35 to f86d893 Compare January 14, 2021 16:41

viktormalik force-pushed the arrays-improvements branch 2 times, most recently from 74e5f78 to 2d1b6f1 Compare January 18, 2021 12:42

danobi reviewed Jan 20, 2021

View reviewed changes

viktormalik added 6 commits January 21, 2021 07:59

Disallow array and struct types inside a tuple

d52477d

Update CHANGELOG for array improvements

340aa3c

viktormalik force-pushed the arrays-improvements branch from 2d1b6f1 to 340aa3c Compare January 21, 2021 06:59

danobi approved these changes Jan 21, 2021

View reviewed changes

mmisono approved these changes Jan 22, 2021

View reviewed changes

fbs merged commit 79d312d into bpftrace:master Jan 24, 2021

viktormalik deleted the arrays-improvements branch January 26, 2021 06:46

viktormalik mentioned this pull request Feb 4, 2021

Cannot read struct into variable #1535

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Array improvements #1656

Array improvements #1656

viktormalik commented Dec 4, 2020 •

edited

Loading

viktormalik commented Dec 4, 2020

danobi Jan 12, 2021

viktormalik Jan 13, 2021

danobi commented Jan 12, 2021

viktormalik commented Jan 14, 2021

mmisono commented Jan 20, 2021

viktormalik commented Jan 20, 2021

danobi Jan 20, 2021

viktormalik Jan 21, 2021

danobi commented Jan 20, 2021

		@@ -2275,10 +2275,32 @@ AllocaInst *CodegenLLVM::getMapKey(Map &map)
		}
		else

Array improvements #1656

Array improvements #1656

Conversation

viktormalik commented Dec 4, 2020 • edited Loading

Checklist

viktormalik commented Dec 4, 2020

danobi Jan 12, 2021

Choose a reason for hiding this comment

viktormalik Jan 13, 2021

Choose a reason for hiding this comment

danobi commented Jan 12, 2021

viktormalik commented Jan 14, 2021

mmisono commented Jan 20, 2021

viktormalik commented Jan 20, 2021

danobi Jan 20, 2021

Choose a reason for hiding this comment

viktormalik Jan 21, 2021

Choose a reason for hiding this comment

danobi commented Jan 20, 2021

viktormalik commented Dec 4, 2020 •

edited

Loading