Small int prep work #1532

fbs · 2020-09-23T19:10:31Z

This is based on #1509, so that will have to land first.

This is some more prep work for #1091. Main things:

break some if nesting in the codegen into separate functions
treat integer dereference like a kptr(uint64 *) as discussed in Define pointer arithmetic #1507. E.g. $x = *123 => $x = * (uint64*)123;
fix a printing bug
Make the SizedType.size attribute private

lgtm-com · 2020-09-23T19:33:13Z

This pull request introduces 1 alert when merging 579a41d into 2ed1ed5 - view on LGTM.com

new alerts:

1 for Missing return statement

fbs · 2020-09-23T21:50:13Z

CHANGELOG.md

@@ -64,6 +64,8 @@ and this project adheres to
  - [#1530](https://github.com/iovisor/bpftrace/pull/1530)
 - Fix pointer arithmetic for positional parameters
  - [#1514](https://github.com/iovisor/bpftrace/pull/1514)
+- Printing of small integers with `printf`
+  - [#1531](https://github.com/iovisor/bpftrace/pull/1531)


sumanthkorikkar · 2020-09-29T15:17:48Z

I am happy to test these changes, once it is rebased on master (after addrspace integration), which could solve few things for us. Thank you

fbs · 2020-10-04T20:54:11Z

ready for review now :)

sumanthkorikkar · 2020-10-06T08:14:44Z

ok. I will check and test this. Thnx

sumanthkorikkar · 2020-10-06T11:35:58Z

I do see the following additional failures.

[ FAILED ] array.array element access - assign to map
[ FAILED ] array.array element access - assign to var
[ FAILED ] intcast.Casting ints
[ FAILED ] variable.32-bit tracepoint arg

sumanthkorikkar · 2020-10-06T13:02:07Z

There is an endianness issue here while printing. I will provide the detailed debug output for analysis. Similarly the case for commit #1509 . Also these failures are related to printf.
[ FAILED ] intptrcast.Casting ints
[ FAILED ] intptrcast.Sum casted value
[ FAILED ] call.printf_argument_alignment

fbs · 2020-10-06T14:46:05Z

Thanks, didn't expect that to happen in this PR. I'll dig into it

sumanthkorikkar · 2020-10-11T10:50:01Z

This seems to be tricky:

In semantic_analyser:
// Promote to 64-bit if it's not an aggregate type
if (!ty.IsAggregate() && !ty.IsTimestampTy())
ty.size = 8;
Then in codegen:
// As these types are 64 bit, we adjust dst size appropriately in codegen
expr_ = b_.CreateIntCast(b_.CreateLoad(dst),
b_.getInt64Ty(),
type.IsSigned());
Now we consider the arg.type.GetIntBitWidth() in get_arg_values(). where size is not 64 bit (For 32 bit / 16 bit val). Hence it fails for Big endian. Previously we considered arg.type.size for this ( which is 64 bit).

This is specific to CreateProbeRead cases

fbs · 2020-10-11T13:07:06Z

I think #1533 will fix that. But might be better to close this and merge that in one go to avoid regressions.

Or i could add a setter for the size attribute here and have it update size_bits when needed. Eventually I hope to get rid of thee option to override size completely.

sumanthkorikkar · 2020-10-11T15:45:43Z

ok. Fine with both. If #1533 is complex and takes long, then we could probably have a setter for the size attribute. Thanks.

fbs · 2020-10-13T22:25:28Z

Updated to iniclude a setter/getter.

sumanthkorikkar · 2020-10-14T07:25:47Z

ok. will check. Thanks.

fbs · 2020-10-14T10:17:51Z

@sumanthkorikkar think I got it fixed :)

sumanthkorikkar · 2020-10-16T15:56:51Z

src/ast/codegen_llvm.cpp

+        // When dereferencing a 32-bit integer, only read in 32-bits, etc.
+        int size = type.IsPtrTy() ? type.GetPointeeTy()->GetSize()
+                                  : type.GetSize();
+        auto as = type.IsPtrTy() ? type.GetAS() : AddrSpace::kernel;


Do we need to set AddrSpace::kernel here?

If this is a valid case: *(int64)somintaddr. Then it would resolve to probe kernel

Yes this should remain backwards compatibility with how integer dereference works currently. As the old probe_read helpers are deprecated we shouldn't rely on them anymore, so we eneed to set the AS here.

I think, this will resolve to probe kernel then, which is not correct. uprobe:./testprogs/intptrcast:fn { $a=*(reg("r15")+160);

I don't think that differs from current behaviour where its also broken. But would be good to discuss in a separate issue

oh, I didn't know bpf_probe_read was dropped in some architectures... If addrspace is none, then bpftrace emits a warning. So, how about keeping addrspace::none in this case and use bpf_probe_read_kernel() if bpf_probe_read() is not available. A user can use uptr() to change the behavior if needed. And maybe we can add a better auto-detect mechanism in another PR.

maybe we need to change this depending on whether arch has overlapping addrspace?
https://github.com/iovisor/bpftrace/blob/e6bbb9a925e405c1ec87790490d81227ac122032/src/ast/irbuilderbpf.cpp#L74-L79

As this PR has some massive refactoring changes and a printf fix I don't want to block this on this discussion/change right now so I've restored the original AS behaviour. For non overlapping arch we should probably get autodetection working properly but that is too much for this PR. We can continue the discussion in #1507 :)

fbs · 2020-10-17T10:16:58Z

Did this fix all the test issues @sumanthkorikkar ?

sumanthkorikkar · 2020-10-17T11:33:56Z

Most of the issues reported works. Thanks. There are few other issues. Checking if they are related to this and will let you know soon.

sumanthkorikkar · 2020-10-18T07:00:29Z

[ FAILED ] usdt."usdt sized arguments" . There is an endianness issue here. But this is not related to this PR. I will send a fix for this today.

For these two failures, Didnt get an idea yet.

[ FAILED ] intptrcast.Casting ints
[ FAILED ] intptrcast.Sum casted value

For these, doing (int64*) instead of (int16*) works.

bpftrace -v -e 'uprobe:./testprogs/intptrcast:fn { $a=*(uint64*)(reg("r15")+160); printf("%d\n", $a); exit();}'-> Works
bpftrace -v -e 'uprobe:./testprogs/intptrcast:fn { $a=*(uint16*)(reg("r15")+160); printf("%d\n", $a); exit();}' -> Does not work

sumanthkorikkar · 2020-10-18T10:02:45Z

#1566 Added for usdt_sized_args

fbs · 2020-10-18T18:04:31Z

For these two failures, Didnt get an idea yet.

* [  FAILED  ] intptrcast.Casting ints

* [  FAILED  ] intptrcast.Sum casted value

For these, doing (int64*) instead of (int16*) works.

* bpftrace -v -e 'uprobe:./testprogs/intptrcast:fn { $a=*(uint64*)(reg("r15")+160); printf("%d\n", $a); exit();}'-> Works

* bpftrace -v -e 'uprobe:./testprogs/intptrcast:fn { $a=*(uint16*)(reg("r15")+160); printf("%d\n", $a); exit();}' -> Does not work

Interesting thanks! Is this a regression compared to master? Can you attach/mail me the -dd output for both? Once we have the VM setup I can dig into this :)

danobi

LGTM (aside from addrspace), just some questions

danobi · 2020-10-14T18:59:51Z

src/ast/codegen_llvm.cpp


    size_t len = std::min(binop.left->type.size, binop.right->type.size);
    expr_ = b_.CreateStrncmp(ctx_,
                             left_string,
                             left_as,
                             right_string,
                             right_as,
-                             len,
+                             len + 1,


Intentional change?

danobi · 2020-10-22T23:40:32Z

src/bpftrace.cpp

@@ -718,28 +718,59 @@ std::vector<std::unique_ptr<IPrintable>> BPFtrace::get_arg_values(const std::vec
    switch (arg.type.type)
    {
      case Type::integer:
-        switch (arg.type.size)
+        if (arg.type.IsSigned())


The reason this fix works is b/c we're forcing a sign extension here, right? If so, can you add that information to the commit message?

@danobi I've removed the confusing cast hack thing and just have Printables handle the actual printing, should be easier to make sense of now.

fbs · 2020-10-27T21:51:38Z

For these two failures, Didnt get an idea yet.
* [  FAILED  ] intptrcast.Casting ints

* [  FAILED  ] intptrcast.Sum casted value

These don't seem related to this PR, happens on my master build too.

sumanthkorikkar · 2020-10-28T05:44:59Z

For these two failures, Didnt get an idea yet.
* [  FAILED  ] intptrcast.Casting ints

* [  FAILED  ] intptrcast.Sum casted value
These don't seem related to this PR, happens on my master build too.

Ok. I will check these 2 failures in another PR. Thanks.

Printf conversion works on s390x.

fbs · 2020-10-30T14:02:34Z

Looks like the debian embedded build broke somehow, doesn't look related

Directly modifying the size field is tricky as other fields (like size_bits) will have to be updated too. By making the field private and hiding it behind a getter and setter that logic will only have to exist in one place. Ideally we disallow modifying size completely but that will require some more changes.

When resizing an integer we have to insert a cast to ensure the sign extension happens (for signed ints), just modifying the `size` isn't enough.

The integer conversion causes signed integers to be printed wrong as they're not correctly sign extended: ``` bpftrace -e 'BEGIN { printf("%d", (int16)-123); exit() ;}' Attaching 1 probe... 65413 ```

fbs mentioned this pull request Sep 23, 2020

Support small integers #1533

Closed

fbs commented Sep 23, 2020

View reviewed changes

fbs force-pushed the 1091_integer_size_part1 branch 3 times, most recently from c7de39d to 883c72d Compare September 25, 2020 14:46

danobi added the do-not-merge Changes are not ready to be merged into master yet label Sep 26, 2020

fbs force-pushed the 1091_integer_size_part1 branch from 883c72d to 438e649 Compare October 4, 2020 13:40

fbs removed the do-not-merge Changes are not ready to be merged into master yet label Oct 4, 2020

fbs force-pushed the 1091_integer_size_part1 branch from 438e649 to f22545b Compare October 13, 2020 22:24

fbs force-pushed the 1091_integer_size_part1 branch 2 times, most recently from fec3690 to 4ef17bf Compare October 14, 2020 10:17

sumanthkorikkar reviewed Oct 16, 2020

View reviewed changes

danobi reviewed Oct 22, 2020

View reviewed changes

mmisono mentioned this pull request Oct 26, 2020

Fix LNOT #1570

Merged

3 tasks

fbs mentioned this pull request Oct 26, 2020

NFC: Split binop codegen to improve readability #1576

Merged

fbs force-pushed the 1091_integer_size_part1 branch 3 times, most recently from 36639bd to c972d32 Compare October 27, 2020 21:33

fbs force-pushed the 1091_integer_size_part1 branch from c972d32 to 40218ba Compare October 29, 2020 19:08

fbs force-pushed the 1091_integer_size_part1 branch 2 times, most recently from e318a46 to 34a5eb7 Compare November 3, 2020 10:46

mmisono approved these changes Nov 3, 2020

View reviewed changes

fbs added 5 commits November 3, 2020 12:47

Integer dereference is always uint64

8139aa8

NFC: Normalize member variable names

9401e49

Move type promotion to codegen

54c4e48

When resizing an integer we have to insert a cast to ensure the sign extension happens (for signed ints), just modifying the `size` isn't enough.

Add signed integer type to printf

2e6a1e4

The integer conversion causes signed integers to be printed wrong as they're not correctly sign extended: ``` bpftrace -e 'BEGIN { printf("%d", (int16)-123); exit() ;}' Attaching 1 probe... 65413 ```

fbs force-pushed the 1091_integer_size_part1 branch from ce58c09 to 2e6a1e4 Compare November 3, 2020 11:48

fbs merged commit 91b0705 into bpftrace:master Nov 3, 2020

fbs deleted the 1091_integer_size_part1 branch August 1, 2021 20:32

Small int prep work #1532

Small int prep work #1532

Conversation

fbs commented Sep 23, 2020 • edited Loading

lgtm-com bot commented Sep 23, 2020

Choose a reason for hiding this comment

sumanthkorikkar commented Sep 29, 2020

fbs commented Oct 4, 2020

sumanthkorikkar commented Oct 6, 2020

sumanthkorikkar commented Oct 6, 2020

sumanthkorikkar commented Oct 6, 2020 • edited Loading

fbs commented Oct 6, 2020

sumanthkorikkar commented Oct 11, 2020 • edited Loading

fbs commented Oct 11, 2020

sumanthkorikkar commented Oct 11, 2020

fbs commented Oct 13, 2020

sumanthkorikkar commented Oct 14, 2020

fbs commented Oct 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbs commented Oct 17, 2020

sumanthkorikkar commented Oct 17, 2020 • edited Loading

sumanthkorikkar commented Oct 18, 2020 • edited Loading

sumanthkorikkar commented Oct 18, 2020

fbs commented Oct 18, 2020

danobi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbs commented Oct 27, 2020

sumanthkorikkar commented Oct 28, 2020 • edited Loading

fbs commented Oct 30, 2020

fbs commented Sep 23, 2020 •

edited

Loading

sumanthkorikkar commented Oct 6, 2020 •

edited

Loading

sumanthkorikkar commented Oct 11, 2020 •

edited

Loading

sumanthkorikkar commented Oct 17, 2020 •

edited

Loading

sumanthkorikkar commented Oct 18, 2020 •

edited

Loading

danobi left a comment •

edited

Loading

sumanthkorikkar commented Oct 28, 2020 •

edited

Loading