allow negative override values

[andrewstrohman]

The user specified override value is stored as an int32 and then cast to uint64. When the override value is negative, this casting creates a very large number. Later when we parse this large constant, we try to store it in a signed integer, which cannot accommodate a value this large.

Hence, write the override return value as a signed value when auto generating the register overrides. This way, ParseAssignment will be able to parse the negative integers.

I tested that negative override worked as expected.

[andrewstrohman]

@olsajiri and @FedeDP I consider this a stopgap, that only fixes the override return functionality. But I don't think that this addresses a larger, underlying problem that affects the generic override register functionality.

See here. I first tried just making off a unint64. While this approach appeared to fix the issue for me, it caused this unit test to fail, which made me realize that we want to allow users to specify negative values for registers.

When we accept negative values, then we cannot accept the upper/larger half of unsigned values without additional logic. Maybe in order to support both negative numbers and the upper half of unsigned numbers when should try to to store into an int first and then try to store into a uint64 next to see if that works.

And then there is the USDT parseConst() code, which might have a similar problem affecting values that are within the larger half of unsigned long, but I'm not sure.

Do either of you have a suggestion for the best path forward here?

[olsajiri]

When we accept negative values, then we cannot accept the upper/larger half of unsigned values without additional logic. Maybe in order to support both negative numbers and the upper half of unsigned numbers when should try to to store into an int first and then try to store into a uint64 next to see if that works.

how about instead of re-using action.ArgError that we use for kprobe override, we could add uint64 action.ArgUprobeError (or some better name) that we will use for uprobe override.. and convert negative values to uint64 and store it to action selector

I did not realize that uprobe override might differ here for kprobes, where we always just return error code.. uprobe needs to support the whole value

[andrewstrohman]

how about instead of re-using action.ArgError that we use for kprobe override, we could add uint64 action.ArgUprobeError (or some better name) that we will use for uprobe override.. and convert negative values to uint64 and store it to action selector

This would fix the issue where we cannot override with any arbitrary 64 bit value. (Currently, we are constrained to values with int32.) This sounds like a good approach to me.

However, the point that I was trying to make was about argRegs, not argError. Here we are storing the scanned value in an int. This means that the user is not able specify this, for example:

        argRegs:
        - "rax=18446744073709551615"

But they could express this same thing with:

        argRegs:
        - "rax=-1"

So, the "issue" is not that the user cannot express an arbitrary value for the register -- it's that they need to know that if they want to express a unsigned long with value 2^63 or greater, they instead need to configure the equivalent negative number, which might cause confusion. That's why I was suggesting trying to store the value in a golang int first, and then uint64 if that doesn't work.

I experimented with trying the hex variant , and this:

        argRegs:
        - "rax=0xffffffffffffffff"

was accepted, but doesn't work as expected.

andy@builder:~/src$ cat test.go
package main

import "fmt"
import "os"

func main() {
        var (
                n   int
                off int
        )

        if n, _ = fmt.Sscanf(os.Args[1], "0x%x", &off); n != 1 {
                if n, _ = fmt.Sscanf(os.Args[1], "%d", &off); n != 1 {
                        fmt.Println("no match")
                        return
                }
        }
        fmt.Println(fmt.Sprintf("%d", off))
}


andy@builder:~/src$ go run test.go 1
1
andy@builder:~/src$ go run test.go 2
2
andy@builder:~/src$ go run test.go -1
-1
andy@builder:~/src$ go run test.go -2
-2
andy@builder:~/src$ go run test.go 18446744073709551615
no match
andy@builder:~/src$ go run test.go 0xffffffffffffffff
0
andy@builder:~/src$ go run test.go 0x7fffffffffffffff
9223372036854775807
andy@builder:~/src$ go run test.go 0x8000000000000000
0
andy@builder:~/src$ go run test.go 0x8000000000000001
0
andy@builder:~/src$ go run test.go 0x8000000000000002
0

What do you think?

[olsajiri]

ok, I see the const parsing is broken.. how about something like this:

--- a/pkg/asm/assignment_linux_amd64.go
+++ b/pkg/asm/assignment_linux_amd64.go
@@ -7,6 +7,7 @@ import (
        "errors"
        "fmt"
        "io"
+       "strconv"
        "strings"
 )
 
@@ -131,18 +132,20 @@ func parseReg(str string, ass *Assignment) error {
 func parseConst(str string, ass *Assignment) error {
 
        var (
-               n   int
-               off int
+               uoff uint64
+               soff int64
+               err  error
        )
 
-       if n, _ = fmt.Sscanf(str, "0x%x", &off); n != 1 {
-               if n, _ = fmt.Sscanf(str, "%d", &off); n != 1 {
+       if uoff, err = strconv.ParseUint(str, 0, 64); err != nil {
+               if soff, err = strconv.ParseInt(str, 0, 64); err != nil {
                        return errNext
                }
+               uoff = uint64(soff)
        }
 
        ass.Type = ASM_ASSIGNMENT_TYPE_CONST
-       ass.Off = uint64(off)
+       ass.Off = uoff
        return nil
 }

[andrewstrohman]

how about something like this

Yes, that addresses my concern. I also think that with that change, we don't need this one any more. So, if you want to create a PR for that, I'll close this one.

ok

Do we need to take a care of a similar problem here?

Could parseRegDeref theoretically have the same problem? Same goes for parseRegOff. What do you think?

I'll double check, but I think reg assignment for const is special (wrt other cases you mentioned), because it's our way overwriting registers, the rest parse out what comes out of compiler and that seems to be always decimal number, also libbpf assumes that