Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gollvm: cannot use unsafe.Pointer as map key type #52861

Open
tianxiaogu opened this issue May 12, 2022 · 3 comments
Open

gollvm: cannot use unsafe.Pointer as map key type #52861

tianxiaogu opened this issue May 12, 2022 · 3 comments
Assignees
@thanm
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone
gollvm

Comments

@tianxiaogu
Copy link

tianxiaogu commented May 12, 2022 •
edited

GOLLVM has a miscompilation bug that can be reproduced with the following test case.

The bug can be reproduced with trunk:

  1. gollvm: https://go.googlesource.com/gollvm/+/0e34e09fc15cde73f1b9974f2a657360abb94b4f
  2. gofrontend: https://go.googlesource.com/gofrontend/+/6a33e7e30c89edc12340dc470b44791bb1066feb

In the following test case, m is a map with unsafe.Pointer as key type. Method test simply adds key p into map m and returns true if the key is not present in the map. So ok1 should be true and ok2 should be false. The gc compiler can produce the correct output whereas GOLLVM miscompiles and leads to wrong output, i.e., both ok1 and ok2 are true.

package main

import (
	"unsafe"
)

func test(m map[unsafe.Pointer]struct{}, p unsafe.Pointer) bool {
	if _, ok := m[p]; ok {
		return false
	}
	m[p] = struct{}{}
	return true
}

func main() {
	m := make(map[unsafe.Pointer]struct{})
	p := unsafe.Pointer(&m)
	ok1 := test(m, p)
	ok2 := test(m, p)
	if !ok1 || ok2 {
		panic("Oops")
	}
}

Below is the LLVM IR obtained via option -tracelevel=1.

define internal i8 @main.test(i8* nest %nest.0, { i64, i8, i8, i16, i32, i8*, i8*, i64, i8* }* %m, i8* %p) #0 !dbg !5 {
entry:
  %m.addr = alloca { i64, i8, i8, i16, i32, i8*, i8*, i64, i8* }*, align 8
  %p.addr = alloca i8*, align 8
  %"$ret0" = alloca i8, align 1
  %ok = alloca i8, align 1
  %tmpv.0 = alloca i8*, align 8, !go_addrtaken !31
  %tmpv.1 = alloca {}*, align 8
  %tmpv.2 = alloca i8, align 1
  %tmpv.3 = alloca { i8*, i8 }, align 8
  %sret.actual.0 = alloca { i8*, i8 }, align 8
  %tmpv.4 = alloca i8*, align 8
  %tmpv.5 = alloca i8, align 1
  %tmpv.6 = alloca i8*, align 8, !go_addrtaken !31
  %tmpv.7 = alloca {}, align 1
  %tmpv.8 = alloca i8*, align 8
  store { i64, i8, i8, i16, i32, i8*, i8*, i64, i8* }* %m, { i64, i8, i8, i16, i32, i8*, i8*, i64, i8* }** %m.addr, align 8
  call void @llvm.dbg.declare(metadata { i64, i8, i8, i16, i32, i8*, i8*, i64, i8* }** %m.addr, metadata !32, metadata !DIExpression()), !dbg !33
  store i8* %p, i8** %p.addr, align 8
  call void @llvm.dbg.declare(metadata i8** %p.addr, metadata !34, metadata !DIExpression()), !dbg !35
  call void @llvm.lifetime.start.p0i8(i64 1, i8* %"$ret0")
  store i8 0, i8* %"$ret0", align 1
  call void @llvm.dbg.declare(metadata i8* %"$ret0", metadata !36, metadata !DIExpression()), !dbg !37
  call void @llvm.lifetime.start.p0i8(i64 1, i8* %ok)
  store i8 0, i8* %ok, align 1
  call void @llvm.dbg.declare(metadata i8* %ok, metadata !38, metadata !DIExpression()), !dbg !40
  %p.ld.0 = load i8*, i8** %p.addr, align 8, !dbg !41
  store i8* %p.ld.0, i8** %tmpv.0, align 8
  %m.ld.0 = load { i64, i8, i8, i16, i32, i8*, i8*, i64, i8* }*, { i64, i8, i8, i16, i32, i8*, i8*, i64, i8* }** %m.addr, align 8, !dbg !42
  %tmpv.0.ld.0 = load i8*, i8** %tmpv.0, align 8, !dbg !43 # <----- additional load
  %cast.22 = bitcast i8* %tmpv.0.ld.0 to i64*, !dbg !43 # <----- following cast
  %deref.ld.0 = load i64, i64* %cast.22, align 8, !dbg !43
  %call.0 = call { i8*, i8 } @runtime.mapaccess2__fast64(i8* nest undef, %_type.0* getelementptr inbounds (%MapType.0, %MapType.0* @type..map_6unsafe_0Pointer_7struct_4_5, i32 0, i32 0), { i64, i8, i8, i16, i32, i8*, i8*, i64, i8* }* %m.ld.0, i64 %deref.ld.0), !dbg !43
...

We can find that there is an additional load that converts i8** into i8* and then i8* is converted into i64*,
which means not the address of p (i.e., type i8**) is converted into i64* but the p (i.e., type i8*) itself is converted into i64* ().

It seems that there is something wrong with the propagation of VarContext. Additional load operations are generated during materialization.
@heschi
Copy link
Contributor

heschi commented May 12, 2022

cc @thanm

@heschi heschi added the NeedsFix The path to resolution is known, but the work has not been done. label May 12, 2022
@heschi heschi added this to the Backlog milestone May 12, 2022
@huanglanzhiguan
Copy link

huanglanzhiguan commented May 18, 2022 •
edited

Source code:

//go:noinline
func foo(m map[unsafe.Pointer]struct{}, p unsafe.Pointer) bool {
  _, ok := m[p]
  return ok
}

and the AST is generated as:

.main.foo(.main.m (g.map_6unsafe_0Pointer_7struct_4_5),.main.p (unsafe.Pointer)) (.main.$ret0 (bool)) : (g.func_8map_6unsafe_0Pointer_7struct_4_5_3unsafe_0Pointer_9_8bool_9)
{
  var .main.ok (bool)  // main.go:9
  {
    tmp.94608400433968 (unsafe.Pointer) = p // main.go:9
    tmp.94608400422240 (g._2struct_4_5) // main.go:9
    tmp.94608400367536 (bool) // main.go:9
    tmp.94608400443200 (g.struct_4_4x_5res0_b_2_4void_5_cres1_bbool_5) // main.go:9
    mapaccess2__fast64((g.map_6unsafe_0Pointer_7struct_4_5),(g.map_6_2_4void_5_7_2_4void_5)(m) ,*((g._2uint64)(&(tmp.94608400433968) ) ) )  // main.go:9
    tmp.94608400472048 = 0@(mapaccess2__fast64((g.map_6unsafe_0Pointer_7struct_4_5),(g.map_6_2_4void_5_7_2_4void_5)(m) ,*((g._2uint64)(&(tmp.94608400433968) ) ) ) ) // main.go:9
    tmp.94608400422240 = (g._2struct_4_5)(tmp.94608400472048)  // main.go:9
    tmp.94608400482960 = 1@(mapaccess2__fast64((g.map_6unsafe_0Pointer_7struct_4_5),(g.map_6_2_4void_5_7_2_4void_5)(m) ,*((g._2uint64)(&(tmp.94608400433968) ) ) ) ) // main.go:9
    tmp.94608400367536 = tmp.94608400482960 // main.go:9
    _ = *(tmp.94608400422240)  // main.go:9
    ok = tmp.94608400367536 // main.go:9
  }
  {
    $ret0 = ok // main.go:10
    return  // main.go:10
  }
}

and the LLVM IR is generated as:

; Function Attrs: noinline
define internal fastcc i8 @main.foo({ i64, i8, i8, i16, i32, i8*, i8*, i64, i8* }* %m, i8* nocapture readonly %p) unnamed_addr #1 !dbg !62 {
entry:
  call void @llvm.dbg.value(metadata { i64, i8, i8, i16, i32, i8*, i8*, i64, i8* }* %m, metadata !80, metadata !DIExpression()), !dbg !81
  call void @llvm.dbg.value(metadata i8* %p, metadata !82, metadata !DIExpression()), !dbg !81
  call void @llvm.dbg.value(metadata i8 0, metadata !83, metadata !DIExpression()), !dbg !81
  call void @llvm.dbg.value(metadata i8 0, metadata !84, metadata !DIExpression()), !dbg !81
  %cast.22 = bitcast i8* %p to i64*, !dbg !86
  %deref.ld.0 = load i64, i64* %cast.22, align 8, !dbg !86
  %call.0 = call { i8*, i8 } @runtime.mapaccess2__fast64(i8* nest undef, %_type.0* getelementptr inbounds (%MapType.0, %MapType.0* @type..map_6unsafe_0Pointer_7struct_4_5, i64 0, i32 0), { i64, i8, i8, i16, i32, i8*, i8*, i64, i8* }* %m, i64 %deref.ld.0), !dbg !86
  %call.0.fca.1.extract = extractvalue { i8*, i8 } %call.0, 1, !dbg !86
  call void @llvm.dbg.value(metadata i8 %call.0.fca.1.extract, metadata !84, metadata !DIExpression()), !dbg !81
  call void @llvm.dbg.value(metadata i8 %call.0.fca.1.extract, metadata !83, metadata !DIExpression()), !dbg !81
  ret i8 %call.0.fca.1.extract, !dbg !87
}

In the IR above, the load instruction "%deref.ld.0 = load i64, i64 %cast.22, align 8, !dbg !86" is incorrect.*

I find that in the function: "Bexpression *Llvm_backend::materializeConversion(Bexpression *convExpr)", the current logic misses the case: "dref(conv(addr(ptr_type), another_ptr_type)", in the code below, the expr's pending context is incorrectly reset(by calling resolveVarContext), so the later dref expression generates an additional load instruction.

  // In the varexpr pending case, decide what to do depending on whether
  // the var is in an lvalue or rvalue context. For something like
  //
  //     var y int32
  //     z = int64(y)
  //
  // we want to force the load of "y" before converting to int64. For
  // an lvalue context, the conversion will be applied to the pointed-to-type
  // as well as the value type.
  if (expr->varExprPending()) {
    bool lvalue = expr->varContext().lvalue();
    if (!lvalue) {
      expr = resolveVarContext(expr);
      val = expr->value();
      valType = val->getType();
    }
    if (lvalue || useCopyForLoadStore(type->type())) {
      llvm::Type *et = expr->btype()->type();
      if (valType->isPointerTy() &&
          valType->getPointerElementType() == et)
        toType = llvm::PointerType::get(toType, addressSpace_);
    }
  }

@ianlancetaylor
Copy link
Contributor

@thanm This test case works as expected with gccgo. Assigning to you to check GoLLVM at your leisure.

@ianlancetaylor ianlancetaylor changed the title gollvm/gofrontend: cannot use unsafe.Pointer as map key type gollvm: cannot use unsafe.Pointer as map key type Jun 28, 2022
@seankhliao seankhliao modified the milestones: Backlog, gollvm Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@thanm thanm

Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
gollvm
Development

No branches or pull requests
6 participants
@tianxiaogu @ianlancetaylor @huanglanzhiguan @seankhliao @thanm @heschi