compiler: zero struct padding during map operations #3437

dgryski · 2023-02-10T00:22:20Z

Fixes #3358

dgryski · 2023-02-10T08:19:41Z

More testing and cleanup required. But otherwise I'm happy with this.

dgryski · 2023-02-10T08:56:32Z

Fixes the test case from #3358.

dgryski · 2023-02-10T15:30:40Z

Still need to come up with a proper test case to add, but I think this is ready for review otherwise.

dgryski · 2023-02-10T15:31:45Z

@anuraaga Can you test this fix in your application?

anuraaga · 2023-02-13T03:15:58Z

Seems to be good, thanks @dgryski!

deadprogram · 2023-02-15T18:19:37Z

@aykevl any comments here before merge?

aykevl

This PR should really get some tests. Perhaps in a new compiler/testdata/map.go, with tests like:

type paddedStruct struct {
    byte a
    int  b
}
func storeToPaddedStruct(m map[paddedStruct]int, key paddedStruct {
    m[key] = 3
}

(Didn't test this code and whether it reproduces the issue, but I think it does).

Also a suggestion: you could use LLVM types throughout instead of Go types. That is, use llvm.Type instead of types.Type for the typ parameter. That might be a little bit more reliable as it certainly catches all types that need explicit zeroing for the padding (in case a Go type is ever implemented as a struct with padding).
The zeroing algorithm itself looks reasonable to me.

compiler/map.go

dgryski · 2023-02-15T23:57:47Z

I'll update this to to use llvm types instead of Go types.

Also I tried to create a unit test for this, and while I could create a case where I was accessing a map with a padded struct, I couldn't cause it to have non-zero padding bytes.

dgryski · 2023-02-16T00:35:20Z

I think I addressed all the issues. Need to run this through the test corpus. Still need to write an actual test.

aykevl · 2023-02-16T02:22:11Z

Thinking some more, I think you also need to zero the end of structs. For example, with this one:

type someStruct struct {
    a float32
    b char
}

This struct is 8 bytes, but only the first 5 are written to.

Also I tried to create a unit test for this, and while I could create a case where I was accessing a map with a padded struct, I couldn't cause it to have non-zero padding bytes.

Unit tests would be great, but are pretty difficult in the case of undefined behavior.
What I was asking was just IR output to show the memzero calls in IR (add a Go file to compiler/testdata, add the filename to compiler/compiler_test.go, and run go test ./compiler -update). That also makes review a bit easier as it clearly shows the calls are inserted when needed.

dgryski · 2023-02-16T02:44:49Z

Thinking some more, I think you also need to zero the end of structs. For example, with this one:

Ah yes. When I was looking at the code I assumed that keySize ended up being the store size, not the alloc size. So indeed I'm missing those bytes. I'll fix this.

dgryski · 2023-02-16T04:51:47Z

Now with a unit test. PTAL.

dgryski · 2023-02-16T19:07:44Z

Passes the test corpus, which is always nice.

aykevl

Have you tried running a few tests under LLVM 14? LLVM 14 still requires pointer types to match, so I think you will hit a few validation errors. (We don't really have such a test in CI, although I think it would be caught if a padded struct gets added to testdata/map.go - maybe that's a good idea anyway).

(The "Suggestion" comments are really just that, style suggestions you can apply if you like them).

aykevl · 2023-02-16T22:27:03Z

compiler/map.go

+
+		for i := 0; i < llvmArrayType.ArrayLength(); i++ {
+			idx := llvm.ConstInt(b.uintptrType, uint64(i), false)
+			elemPtr := b.CreateGEP(llvmArrayType, ptr, []llvm.Value{idx}, "")


You can use CreateInBoundsGEP here (and other places in this PR), because you know the GEP is going to be in bounds. ("in bounds", as I understand it, basically means you are 100% sure the pointer is still pointing inside the allocated object - in this case, the alloca).

Also, I believe you need to add a zero index as the first index parameter to the GEP, like here:

tinygo/compiler/compiler.go

Lines 2020 to 2021 in 0d56dee

zero := llvm.ConstInt(b.ctx.Int32Type(), 0, false)

ptr := b.CreateInBoundsGEP(arrayType, alloca, []llvm.Value{zero, index}, "index.gep")

More information: https://llvm.org/docs/GetElementPtr.html

aykevl · 2023-02-16T22:39:36Z

compiler/map.go

+		numFields := llvmStructType.StructElementTypesCount()
+		llvmElementTypes := llvmStructType.StructElementTypes()
+
+		for i := 0; i < llvmStructType.StructElementTypesCount(); i++ {


Suggestion, I think this looks better:

Suggested change

for i := 0; i < llvmStructType.StructElementTypesCount(); i++ {

for i := 0; i < numFields; i++ {

Or even:

Suggested change

for i := 0; i < llvmStructType.StructElementTypesCount(); i++ {

for i, llvmElemType := range llvmElementTypes {

aykevl · 2023-02-16T22:42:22Z

compiler/map.go

+		for i := 0; i < llvmStructType.StructElementTypesCount(); i++ {
+			offset := b.targetData.ElementOffset(llvmStructType, i)
+			llvmOffset := llvm.ConstInt(b.uintptrType, offset, false)
+			elemPtr := b.CreateGEP(b.ctx.Int8Type(), ptr, []llvm.Value{llvmOffset}, "")


This line will likely lead to a verifier failure in LLVM 14. To fix it, you need to cast ptr to an i8*. You can use b.CreateBitCast(ptr, b.i8ptrType, "") to cast the pointer type. And cast back to the pointer element type after the GEP.

aykevl · 2023-02-16T22:49:25Z

compiler/map.go

+				if storeSize := b.targetData.TypeStoreSize(llvmElemType); offset+storeSize != nextOffset {
+					n := llvm.ConstInt(b.uintptrType, nextOffset-(offset+storeSize), false)


Suggestion. Doing the calculation only once may be easier to read.

Suggested change

if storeSize := b.targetData.TypeStoreSize(llvmElemType); offset+storeSize != nextOffset {

n := llvm.ConstInt(b.uintptrType, nextOffset-(offset+storeSize), false)

gap := nextOffset-(offset+b.targetData.TypeStoreSize(llvmElemType))

if gap > 0 {

n := llvm.ConstInt(b.uintptrType, gap, false)

aykevl · 2023-02-16T22:51:11Z

compiler/map.go

+				if storeSize := b.targetData.TypeStoreSize(llvmElemType); offset+storeSize != nextOffset {
+					n := llvm.ConstInt(b.uintptrType, nextOffset-(offset+storeSize), false)
+					llvmSize := llvm.ConstInt(b.uintptrType, storeSize, false)
+					paddingStart := b.CreateGEP(b.ctx.Int8Type(), elemPtr, []llvm.Value{llvmSize}, "")


Another GEP that will likely fail in LLVM 14.

aykevl · 2023-02-16T22:56:22Z

compiler/map.go

+		// zero any padding bytes between end of last field and end of struct
+		allocSize := b.targetData.TypeAllocSize(llvmStructType)
+		lastElemOffset := b.targetData.ElementOffset(llvmStructType, numFields-1)
+		lastElemStore := b.targetData.TypeStoreSize(llvmElementTypes[numFields-1])
+
+		if allocSize != lastElemOffset+lastElemStore {
+			n := llvm.ConstInt(b.uintptrType, allocSize-(lastElemOffset+lastElemStore), false)
+			llvmSize := llvm.ConstInt(b.uintptrType, lastElemOffset+lastElemStore, false)
+			paddingStart := b.CreateGEP(b.ctx.Int8Type(), ptr, []llvm.Value{llvmSize}, "")
+			b.createRuntimeCall("memzero", []llvm.Value{paddingStart, n}, "")
+		}


You could consider merging these in the for loop above, with logic like this (in pseudocode):

if last field: nextOffset = one-past-the-end of the struct else: nextOffset = start offset of next field if offset + TypeAllocSize(field) != nextOffset: zero padding

Not sure it's more readable though.

aykevl · 2023-02-16T23:22:57Z

compiler/testdata/zeromap.go

+func main() {
+	m := make(map[hasPadding]int)
+	var s hasPadding
+
+	for i := 0; i < 10; i++ {
+		s.b1 = i&1 == 0
+		s.i = i
+		s.b2 = i&1 == 1
+		m[s]++
+	}
+	println(len(m))
+}


Suggestion: use the following code to test:

func testZeroGet(m map[hasPadding]int, s hasPadding) int { return m[s] } func testZeroSet(m map[hasPadding]int, s hasPadding) { m[s] = 5 }

This makes zeromap.ll smaller and more readable while still testing the important parts (the calls to runtime.memzero). For example, testZeroGet compiles to:

define hidden i32 @main.testZeroGet(ptr dereferenceable_or_null(40) %m, i1 %s.b1, i32 %s.i, i1 %s.b2, ptr %context) unnamed_addr #1 { entry: %hashmap.key = alloca %main.hasPadding, align 8 %hashmap.value = alloca i32, align 4 %s = alloca %main.hasPadding, align 8 %0 = insertvalue %main.hasPadding zeroinitializer, i1 %s.b1, 0 %1 = insertvalue %main.hasPadding %0, i32 %s.i, 1 %2 = insertvalue %main.hasPadding %1, i1 %s.b2, 2 %stackalloc = alloca i8, align 1 store %main.hasPadding zeroinitializer, ptr %s, align 8 call void @runtime.trackPointer(ptr nonnull %s, ptr nonnull %stackalloc, ptr undef) #3 store %main.hasPadding %2, ptr %s, align 8 call void @llvm.lifetime.start.p0(i64 4, ptr nonnull %hashmap.value) call void @llvm.lifetime.start.p0(i64 12, ptr nonnull %hashmap.key) store %main.hasPadding %2, ptr %hashmap.key, align 8 %3 = getelementptr inbounds i8, ptr %hashmap.key, i32 1 call void @runtime.memzero(ptr nonnull %3, i32 3, ptr undef) #3 %4 = getelementptr inbounds i8, ptr %hashmap.key, i32 9 call void @runtime.memzero(ptr nonnull %4, i32 3, ptr undef) #3 %5 = call i1 @runtime.hashmapBinaryGet(ptr %m, ptr nonnull %hashmap.key, ptr nonnull %hashmap.value, i32 4, ptr undef) #3 call void @llvm.lifetime.end.p0(i64 12, ptr nonnull %hashmap.key) %6 = load i32, ptr %hashmap.value, align 4 call void @llvm.lifetime.end.p0(i64 4, ptr nonnull %hashmap.value) ret i32 %6 }

Also, it might be worth testing an array as well (it could just be a [2]hasPadding).

dgryski · 2023-02-17T20:41:38Z

Trying to get the CreateBitCast stuff to work and I keep getting Invalid bitcast, so that's what I'm fighting with today.

Fixes tinygo-org#3358

dgryski · 2023-02-18T03:08:06Z

OK. Switched to CreateInBoundsGEP, switched the upper limit of the for loop to numFields, simplified the unit test, combined last-field zero logic, added zeros to the GEP calls. I couldn't figure out how to use the CreateBitCast without causing verification failures. .

aykevl · 2023-02-19T22:05:05Z

Did a quick check and this PR as-is is indeed not working in LLVM 14:

Call parameter type does not match function signature!
  %5 = getelementptr inbounds i8*, i1* %4, i64 1, !dbg !43
 i8*  call void @runtime.memzero(i8** %5, i64 7, i8* undef), !dbg !43

Will take a more in-depth look later.
I'm pretty sure you can reproduce this on MacOS (brew install llvm@14, go install -tags=llvm14).

dgryski · 2023-02-21T23:06:06Z

I have LLVM 14 from checking out the reflect fixes. I'll look at this now.

aykevl · 2023-02-23T23:50:41Z

Didn't fully test this but it seems to fix all the verification failures for me in LLVM 14:

--- a/compiler/map.go
+++ b/compiler/map.go
@@ -325,7 +325,14 @@ func (b *builder) zeroUndefBytes(llvmType llvm.Type, ptr llvm.Value) error {
                        if fieldEndOffset != nextOffset {
                                n := llvm.ConstInt(b.uintptrType, nextOffset-fieldEndOffset, false)
                                llvmStoreSize := llvm.ConstInt(b.uintptrType, storeSize, false)
-                               paddingStart := b.CreateInBoundsGEP(b.i8ptrType, elemPtr, []llvm.Value{llvmStoreSize}, "")
+                               gepPtr := elemPtr
+                               if gepPtr.Type() != b.i8ptrType {
+                                       gepPtr = b.CreateBitCast(gepPtr, b.i8ptrType, "") // LLVM 14
+                               }
+                               paddingStart := b.CreateInBoundsGEP(b.ctx.Int8Type(), gepPtr, []llvm.Value{llvmStoreSize}, "")
+                               if paddingStart.Type() != b.i8ptrType {
+                                       paddingStart = b.CreateBitCast(paddingStart, b.i8ptrType, "") // LLVM 14
+                               }
                                b.createRuntimeCall("memzero", []llvm.Value{paddingStart, n}, "")
                        }
                }

aykevl · 2023-02-24T16:29:32Z

Explanation:

if gepPtr.Type() != b.i8ptrType {

Check for LLVM 14. In LLVM 14, every pointer is different (with different element types), while in LLVM 15 all pointers in the same address space are the same.

gepPtr = b.CreateBitCast(gepPtr, b.i8ptrType, "")

Convert to i8*.

dgryski · 2023-02-24T19:53:23Z

PTAL.

dgryski force-pushed the dgryski/zero-struct-padding branch from 1ca9e1e to ace35c5 Compare February 10, 2023 08:41

dgryski marked this pull request as ready for review February 10, 2023 15:30

dgryski requested a review from aykevl February 10, 2023 15:56

aykevl reviewed Feb 15, 2023

View reviewed changes

compiler/map.go Outdated Show resolved Hide resolved

compiler/map.go Outdated Show resolved Hide resolved

compiler/map.go Outdated Show resolved Hide resolved

dgryski force-pushed the dgryski/zero-struct-padding branch from ace35c5 to b50a471 Compare February 16, 2023 00:34

dgryski force-pushed the dgryski/zero-struct-padding branch from b50a471 to 0b873f5 Compare February 16, 2023 01:27

dgryski force-pushed the dgryski/zero-struct-padding branch 4 times, most recently from 7b537f1 to 1e40948 Compare February 16, 2023 04:51

aykevl reviewed Feb 16, 2023

View reviewed changes

dgryski force-pushed the dgryski/zero-struct-padding branch from 4ef3a42 to 56c05c3 Compare February 17, 2023 23:10

compiler: zero struct padding during map operations

da19745

Fixes tinygo-org#3358

dgryski force-pushed the dgryski/zero-struct-padding branch from 5df309c to da19745 Compare February 18, 2023 03:05

compiler: llvm14 fixes for zero undef padding bytes

97320cf

aykevl approved these changes Feb 25, 2023

View reviewed changes

aykevl merged commit 4766217 into tinygo-org:dev Feb 25, 2023

dgryski deleted the dgryski/zero-struct-padding branch March 31, 2023 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: zero struct padding during map operations #3437

compiler: zero struct padding during map operations #3437

dgryski commented Feb 10, 2023

dgryski commented Feb 10, 2023

dgryski commented Feb 10, 2023

dgryski commented Feb 10, 2023

dgryski commented Feb 10, 2023

anuraaga commented Feb 13, 2023

deadprogram commented Feb 15, 2023

aykevl left a comment

dgryski commented Feb 15, 2023

dgryski commented Feb 16, 2023

aykevl commented Feb 16, 2023

dgryski commented Feb 16, 2023

dgryski commented Feb 16, 2023

dgryski commented Feb 16, 2023

aykevl left a comment

aykevl Feb 16, 2023

aykevl Feb 16, 2023

aykevl Feb 16, 2023

aykevl Feb 16, 2023

aykevl Feb 16, 2023

aykevl Feb 16, 2023

aykevl Feb 16, 2023

dgryski commented Feb 17, 2023

dgryski commented Feb 18, 2023

aykevl commented Feb 19, 2023

dgryski commented Feb 21, 2023

aykevl commented Feb 23, 2023

aykevl commented Feb 24, 2023

dgryski commented Feb 24, 2023

	zero := llvm.ConstInt(b.ctx.Int32Type(), 0, false)
	ptr := b.CreateInBoundsGEP(arrayType, alloca, []llvm.Value{zero, index}, "index.gep")

	for i := 0; i < llvmStructType.StructElementTypesCount(); i++ {
	for i := 0; i < numFields; i++ {

	for i := 0; i < llvmStructType.StructElementTypesCount(); i++ {
	for i, llvmElemType := range llvmElementTypes {

		if storeSize := b.targetData.TypeStoreSize(llvmElemType); offset+storeSize != nextOffset {
		n := llvm.ConstInt(b.uintptrType, nextOffset-(offset+storeSize), false)

-				if storeSize := b.targetData.TypeStoreSize(llvmElemType); offset+storeSize != nextOffset {
-					n := llvm.ConstInt(b.uintptrType, nextOffset-(offset+storeSize), false)
+				gap := nextOffset-(offset+b.targetData.TypeStoreSize(llvmElemType))
+				if gap > 0 {
+					n := llvm.ConstInt(b.uintptrType, gap, false)

compiler: zero struct padding during map operations #3437

compiler: zero struct padding during map operations #3437

Conversation

dgryski commented Feb 10, 2023

dgryski commented Feb 10, 2023

dgryski commented Feb 10, 2023

dgryski commented Feb 10, 2023

dgryski commented Feb 10, 2023

anuraaga commented Feb 13, 2023

deadprogram commented Feb 15, 2023

aykevl left a comment

Choose a reason for hiding this comment

dgryski commented Feb 15, 2023

dgryski commented Feb 16, 2023

aykevl commented Feb 16, 2023

dgryski commented Feb 16, 2023

dgryski commented Feb 16, 2023

dgryski commented Feb 16, 2023

aykevl left a comment

Choose a reason for hiding this comment

aykevl Feb 16, 2023

Choose a reason for hiding this comment

aykevl Feb 16, 2023

Choose a reason for hiding this comment

aykevl Feb 16, 2023

Choose a reason for hiding this comment

aykevl Feb 16, 2023

Choose a reason for hiding this comment

aykevl Feb 16, 2023

Choose a reason for hiding this comment

aykevl Feb 16, 2023

Choose a reason for hiding this comment

aykevl Feb 16, 2023

Choose a reason for hiding this comment

dgryski commented Feb 17, 2023

dgryski commented Feb 18, 2023

aykevl commented Feb 19, 2023

dgryski commented Feb 21, 2023

aykevl commented Feb 23, 2023

aykevl commented Feb 24, 2023

dgryski commented Feb 24, 2023