Use unsafe.Add everywhere

[aykevl]

1. Replace all unsafe.Pointer(uintptr(v) + idx) instances with just unsafe.Add(v, idx).
2. Remove the optimization that was previously needed to make that case more efficient. This reduces code complexity.

[dgryski]

I have a semgrep rule that covers this:

rules:
  - id: use-unsafe-add
    patterns:
        - pattern-either:
              - pattern: |
                    (*$TYPE)(unsafe.Pointer(uintptr(unsafe.Pointer($P)) + $LEN))
    message: "Use unsafe.Add(unsafe.Pointer($P), $LEN)"
    fix: "(*$TYPE)(unsafe.Add(unsafe.Pointer($P), $LEN))"
    languages: [go]
    severity: ERROR

We can run this over the codebase and tweak it to make sure it hits all the different cases. I was planning on this doing after my reflect work settled down to avoid the conflicts in reflect/* .

Edit: Trying to get my rule to work and I'm running into issues with semgrep stealing )'s.

[aykevl]

We can run this over the codebase and tweak it to make sure it hits all the different cases.

It won't catch all of them though, especially the ones in src/runtime/hashmap.go (it would have to look over multiple lines, never used semgrep so I don't know whether it's capable to do that). What I did was that I modified the compiler a bit to output an error instead of applying the optimization that's in the second commit. Then, compiling all smoke tests will catch all instances of this optimization.

I was planning on this doing after my reflect work settled down to avoid the conflicts in reflect/* .

I see. This is just some code I already had in a local branch but didn't push because I wanted to wait for the #2640 refactor (which is now merged). I figured I could make a PR now before it starts drifting again.

[dgryski]

My goal for the rule was to catch places where we could use unsafe.Add, not places that triggered your optimization. As such, it was a slightly wider net:

rules:
  - id: use-unsafe-add-1
    patterns:
        - pattern-either:
              - pattern: |
                  (*$TYPE)(unsafe.Pointer((uintptr(unsafe.Pointer($P)) + $LEN)))
              - pattern: |
                  (*$TYPE)(unsafe.Pointer(uintptr(unsafe.Pointer($P)) + $LEN))
    fix: "(*$TYPE)(unsafe.Add(unsafe.Pointer($P), $LEN)"
    message: "use unsafe.Add"
    languages: [go]
    severity: ERROR
  - id: use-unsafe-add-2
    patterns:
              - pattern: |
                    (*$TYPE)(unsafe.Pointer(uintptr($P) + $LEN))
    fix: "(*$TYPE)(unsafe.Add($P, $LEN)"
    message: "use unsafe.Add"
    languages: [go]
    severity: ERROR
  - id: use-unsafe-add-3
    patterns:
              - pattern: |
                    unsafe.Pointer(uintptr(unsafe.Pointer($P)) + $LEN)
              - pattern: |
                    unsafe.Pointer((uintptr(unsafe.Pointer($P)) + $LEN))
    fix: "unsafe.Add(unsafe.Pointer($P), $LEN)"
    message: "use unsafe.Add"
    languages: [go]
    severity: ERROR
  - id: use-unsafe-add-4
    patterns:
              - pattern: |
                    unsafe.Pointer(uintptr($P) + $LEN)
    fix: "unsafe.Add($P, $LEN)"
    message: "use unsafe.Add"
    languages: [go]
    severity: ERROR

As I said, this currently doesn't work because of a known bug in semgrep. I'm going to see if I can write something to handle all the fixups of the missing parens. I tried comby to do the rewrites but it was too slow, and ruleguard doesn't work because the tinygo stdlib code doesn't build for it.

[deadprogram]

Seems like this PR introduces a problem with the MaixBiT

https://github.com/tinygo-org/tinygo/pull/3503/checks?check_run_id=11716348330

I am able to flash it with code from other branches.

[aykevl]

@deadprogram apparently I tried to also merge runtime_tinygoriscv.go and runtime_tinygoriscv64.go in the same commit, but didn't do so correctly. I've undone that change - it could still be a nice improvement but it's better to do unrelated to unsafe.Add changes.

My goal for the rule was to catch places where we could use unsafe.Add, not places that triggered your optimization. As such, it was a slightly wider net:

Is it? Are there any cases it finds that aren't in this PR?

[deadprogram]

This is a good change, thanks @aykevl

[dgryski]

Running my auto-convert semgrep rules on dev after this was merged also found the following:

diff --git a/src/machine/machine_atsamd21.go b/src/machine/machine_atsamd21.go
index 7a5a20c7..77f9ebdd 100644
--- a/src/machine/machine_atsamd21.go
+++ b/src/machine/machine_atsamd21.go
@@ -294,7 +294,7 @@ func InitADC() {
        // #define ADC_FUSES_LINEARITY_1_Msk   (0x7u << ADC_FUSES_LINEARITY_1_Pos)
        // #define ADC_FUSES_LINEARITY_1(value) ((ADC_FUSES_LINEARITY_1_Msk & ((value) << ADC_FUSES_LINEARITY_1_Pos)))

-       biasFuse := *(*uint32)(unsafe.Pointer(uintptr(0x00806020) + 4))
+       biasFuse := *((*uint32)(unsafe.Add(0x00806020, 4)
        bias := uint16(biasFuse>>3) & uint16(0x7)

        // ADC Linearity bits 4:0
@@ -302,7 +302,7 @@ func InitADC() {
        linearity := uint16(linearity0Fuse>>27) & uint16(0x1f)

        // ADC Linearity bits 7:5
-       linearity1Fuse := *(*uint32)(unsafe.Pointer(uintptr(0x00806020) + 4))
+       linearity1Fuse := *((*uint32)(unsafe.Add(0x00806020, 4)
        linearity |= uint16(linearity1Fuse) & uint16(0x7) << 5

        // set calibration
diff --git a/src/machine/machine_atsamd21_usb.go b/src/machine/machine_atsamd21_usb.go
index d170c917..c7fe7b25 100644
--- a/src/machine/machine_atsamd21_usb.go
+++ b/src/machine/machine_atsamd21_usb.go
@@ -88,7 +88,7 @@ func handlePadCalibration() {
        // #define USB_FUSES_TRIM_Msk          (0x7u << USB_FUSES_TRIM_Pos)
        // #define USB_FUSES_TRIM(value)       ((USB_FUSES_TRIM_Msk & ((value) << USB_FUSES_TRIM_Pos)))
        //
-       fuse := *(*uint32)(unsafe.Pointer(uintptr(0x00806020) + 4))
+       fuse := *((*uint32)(unsafe.Add(0x00806020, 4)
        calibTransN := uint16(fuse>>13) & uint16(0x1f)
        calibTransP := uint16(fuse>>18) & uint16(0x1f)
        calibTrim := uint16(fuse>>23) & uint16(0x7)
diff --git a/src/machine/machine_atsamd51_usb.go b/src/machine/machine_atsamd51_usb.go
index b3f570ac..8256c863 100644
--- a/src/machine/machine_atsamd51_usb.go
+++ b/src/machine/machine_atsamd51_usb.go
@@ -91,7 +91,7 @@ func handlePadCalibration() {
        // #define USB_FUSES_TRIM_Msk          (0x7u << USB_FUSES_TRIM_Pos)
        // #define USB_FUSES_TRIM(value)       ((USB_FUSES_TRIM_Msk & ((value) << USB_FUSES_TRIM_Pos)))
        //
-       fuse := *(*uint32)(unsafe.Pointer(uintptr(0x00806020) + 4))
+       fuse := *((*uint32)(unsafe.Add(0x00806020, 4)
        calibTransN := uint16(fuse>>13) & uint16(0x1f)
        calibTransP := uint16(fuse>>18) & uint16(0x1f)
        calibTrim := uint16(fuse>>23) & uint16(0x7)
diff --git a/src/machine/machine_nrf.go b/src/machine/machine_nrf.go
index 3b07a897..eecaced4 100644
--- a/src/machine/machine_nrf.go
+++ b/src/machine/machine_nrf.go
@@ -401,7 +401,7 @@ func (f flashBlockDevice) ReadAt(p []byte, off int64) (n int, err error) {
                return 0, errFlashCannotReadPastEOF
        }

-       data := unsafe.Slice((*byte)(unsafe.Pointer(FlashDataStart()+uintptr(off))), len(p))
+       data := unsafe.Slice((*byte)(unsafe.Add(FlashDataStart(), uintptr(off))), len(p))
        copy(p, data)

        return len(p), nil
diff --git a/src/machine/machine_stm32_flash.go b/src/machine/machine_stm32_flash.go
index 00ec718c..3fcf4c69 100644
--- a/src/machine/machine_stm32_flash.go
+++ b/src/machine/machine_stm32_flash.go
@@ -23,7 +23,7 @@ func (f flashBlockDevice) ReadAt(p []byte, off int64) (n int, err error) {
                return 0, errFlashCannotReadPastEOF
        }

-       data := unsafe.Slice((*byte)(unsafe.Pointer(FlashDataStart()+uintptr(off))), len(p))
+       data := unsafe.Slice((*byte)(unsafe.Add(FlashDataStart(), uintptr(off))), len(p))
        copy(p, data)

        return len(p), nil
diff --git a/src/runtime/dynamic_arm64.go b/src/runtime/dynamic_arm64.go
index e167f6f2..54fde3a9 100644
--- a/src/runtime/dynamic_arm64.go
+++ b/src/runtime/dynamic_arm64.go
@@ -38,7 +38,7 @@ func dynamicLoader(base uintptr, dyn *dyn64) {
        for dyn.Tag != dtNULL {
                switch dyn.Tag {
                case dtRELA:
-                       rela = (*rela64)(unsafe.Pointer(base + uintptr(dyn.Val)))
+                       rela = (*rela64)(unsafe.Add(base, uintptr(dyn.Val)))
                case dtRELASZ:
                        relasz = uint64(dyn.Val) / uint64(unsafe.Sizeof(rela64{}))
                }
@@ -62,7 +62,7 @@ func dynamicLoader(base uintptr, dyn *dyn64) {
                        if debugLoader {
                                println("relocating ", uintptr(rela.Addend), " to ", base+uintptr(rela.Addend))
                        }
-                       ptr := (*uint64)(unsafe.Pointer(base + uintptr(rela.Off)))
+                       ptr := (*uint64)(unsafe.Add(base, uintptr(rela.Off)))
                        *ptr = uint64(base + uintptr(rela.Addend))
                default:
                        if debugLoader {
diff --git a/src/runtime/os_darwin.go b/src/runtime/os_darwin.go
index 6584d6ca..f35db9fe 100644
--- a/src/runtime/os_darwin.go
+++ b/src/runtime/os_darwin.go
@@ -79,7 +79,7 @@ func markGlobals() {
        // pointer to that struct in advance.
        var offset uintptr
        var hasOffset bool
-       cmd := (*segmentLoadCommand)(unsafe.Pointer(uintptr(unsafe.Pointer(&libc_mh_execute_header)) + unsafe.Sizeof(machHeader{})))
+       cmd := ((*segmentLoadCommand)(unsafe.Add(unsafe.Pointer(&libc_mh_execute_header), unsafe.Sizeof(machHeader{}))
        for i := libc_mh_execute_header.ncmds; i != 0; i-- {
                if cmd.cmd == LC_SEGMENT_64 {
                        if cmd.fileoff == 0 && cmd.nsects != 0 {
diff --git a/src/runtime/os_linux.go b/src/runtime/os_linux.go
index 0e6ad474..766950da 100644
--- a/src/runtime/os_linux.go
+++ b/src/runtime/os_linux.go
@@ -87,7 +87,7 @@ func markGlobals() {
                PF_W    = 0x2 // program flag: write access
        )

-       headerPtr := unsafe.Pointer(uintptr(unsafe.Pointer(&ehdr_start)) + ehdr_start.phoff)
+       headerPtr := unsafe.Add(unsafe.Pointer(&ehdr_start), ehdr_start.phoff)
        for i := 0; i < int(ehdr_start.phnum); i++ {
                // Look for a writable segment and scan its contents.
                // There is a little bit of duplication here, which is unfortunate. But
diff --git a/src/runtime/os_windows.go b/src/runtime/os_windows.go
index 7e060497..b4b0be0d 100644
--- a/src/runtime/os_windows.go
+++ b/src/runtime/os_windows.go
@@ -79,7 +79,7 @@ func markGlobals() {
        }

        // Iterate through sections.
-       section := (*peSection)(unsafe.Pointer(uintptr(unsafe.Pointer(pe)) + uintptr(pe.sizeOfOptionalHeader) + unsafe.Sizeof(peHeader{})))
+       section := (*peSection)(unsafe.Add(uintptr(unsafe.Pointer(pe)) + uintptr(pe.sizeOfOptionalHeader), unsafe.Sizeof(peHeader{})))
        for i := 0; i < int(pe.numberOfSections); i++ {
                if section.characteristics&IMAGE_SCN_MEM_WRITE != 0 {
                        // Found a writable section. Scan the entire section for roots.
diff --git a/src/runtime/runtime_atsamd21.go b/src/runtime/runtime_atsamd21.go
index 166df214..25608edd 100644
--- a/src/runtime/runtime_atsamd21.go
+++ b/src/runtime/runtime_atsamd21.go
@@ -70,7 +70,7 @@ func initClocks() {
        // #define SYSCTRL_FUSES_OSC32K_CAL(value) ((SYSCTRL_FUSES_OSC32K_CAL_Msk & ((value) << SYSCTRL_FUSES_OSC32K_CAL_Pos)))
        // u32_t fuse = *(u32_t *)FUSES_OSC32K_CAL_ADDR;
        // u32_t calib = (fuse & FUSES_OSC32K_CAL_Msk) >> FUSES_OSC32K_CAL_Pos;
-       fuse := *(*uint32)(unsafe.Pointer(uintptr(0x00806020) + 4))
+       fuse := *((*uint32)(unsafe.Add(0x00806020, 4)
        calib := (fuse & uint32(0x7f<<6)) >> 6

        // SYSCTRL_OSC32K_CALIB(calib) |

[aykevl]

Interesting, those are mostly constant integer addition which would indeed not be caught by me.

Can you make a PR to fix those remaining bits?