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

incorrect cmpxchg signature #230

Closed
db7 opened this issue Aug 25, 2023 · 4 comments · Fixed by #231
Closed

incorrect cmpxchg signature #230

db7 opened this issue Aug 25, 2023 · 4 comments · Fixed by #231

Comments

@db7
Copy link
Contributor

db7 commented Aug 25, 2023

Here is a long standing bug we have fixed internally. I can create a PR if desired.

Description

The cmpxchg return value has I1 type, but the package defines it as I8 (see here ). That causes clang to fail to compile generated files that contain cmpxchg.

How to reproduce

Save the following code as input.ll :

; ModuleID = 'tests/cmpxchg.c'
source_filename = "tests/cmpxchg.c"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-linux-gnu"

@shared = common dso_local global i32 0, align 4
@expected = common dso_local global i32 0, align 4

; Function Attrs: noinline nounwind optnone uwtable
define dso_local i32 @main() #0 {
  %1 = alloca i32, align 4
  %2 = alloca i32, align 4
  %3 = alloca i8, align 1
  store i32 0, i32* %1, align 4
  store i32 1, i32* %2, align 4
  %4 = load i32, i32* @expected, align 4
  %5 = load i32, i32* %2, align 4
  %6 = cmpxchg i32* @shared, i32 %4, i32 %5 seq_cst seq_cst
  %7 = extractvalue { i32, i1 } %6, 0
  %8 = extractvalue { i32, i1 } %6, 1
  br i1 %8, label %10, label %9

9:                                                ; preds = %0
  store i32 %7, i32* @expected, align 4
  br label %10

10:                                               ; preds = %9, %0
  %11 = zext i1 %8 to i8
  store i8 %11, i8* %3, align 1
  %12 = load i8, i8* %3, align 1
  %13 = trunc i8 %12 to i1
  ret i32 0
}

attributes #0 = { noinline nounwind optnone uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="all" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }

!llvm.module.flags = !{!0}
!llvm.ident = !{!1}

!0 = !{i32 1, !"wchar_size", i32 4}
!1 = !{!"clang version 10.0.0-4ubuntu1 "}

Now use this small program with any version of package:

package main

import (
        "fmt"
        "os"

        "github.com/llir/llvm/asm"
)

func main() {
        mod, err := asm.ParseFile("input.ll")
        if err != nil {
                panic(err)
        }

        fp, err := os.Create("output.ll")
        if err != nil {
                panic(err)
        }
        defer fp.Close()
        fmt.Fprint(fp, mod)
}

Finally, try to compile the generated file:

$ clang output.ll
output.ll:18:32: error: '%6' defined with type '{ i32, i1 }' but expected '{ i32, i8 }'
        %7 = extractvalue { i32, i8 } %6, 0
                                      ^
1 error generated.

Fix

--- a/vendor/github.com/llir/llvm/asm/inst_memory.go
+++ b/vendor/github.com/llir/llvm/asm/inst_memory.go
@@ -44,7 +44,7 @@ func (fgen *funcGen) newCmpXchgInst(ident ir.LocalIdent, old *ast.CmpXchgInst) (
        if err != nil {
                return nil, errors.WithStack(err)
        }
-       typ := types.NewStruct(oldType, types.I8)
+       typ := types.NewStruct(oldType, types.I1)
        return &ir.InstCmpXchg{LocalIdent: ident, Typ: typ}, nil
 }
@dannypsnl
Copy link
Member

Sure, please send PR

db7 added a commit to db7/llvm that referenced this issue Aug 25, 2023
Resolves llir#230.

Signed-off-by: Diogo Behrens <diogo.behrens@huawei.com>
mewmew pushed a commit that referenced this issue Aug 25, 2023
Resolves #230.

Signed-off-by: Diogo Behrens <diogo.behrens@huawei.com>
@mewmew
Copy link
Member

mewmew commented Aug 25, 2023

@db7 thanks for the detailed bug report and also submitting the one-line fix! You're indeed correct that the second field of the struct returned by the cmpxchg should be a boolean of type i1.

Of curiosity, in what kind of projects are you using the llir/llvm package? : )

Wish you all the best and happy coding.

Cheers,
Robin

@db7
Copy link
Contributor Author

db7 commented Mar 17, 2024

Of curiosity, in what kind of projects are you using the llir/llvm package? : )

https://github.com/open-s4c/vsyncer

@mewmew
Copy link
Member

mewmew commented Mar 17, 2024

Of curiosity, in what kind of projects are you using the llir/llvm package? : )

https://github.com/open-s4c/vsyncer

Hi Diogo,

Really cool project! Thanks for releasing it as open source.

Cheerful regards,
Robin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants