Skip to content

_Complex _BitInt is inconsistently/incorrectly lowered #119352

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

Closed
bevin-hansson opened this issue Dec 10, 2024 · 7 comments
Closed

_Complex _BitInt is inconsistently/incorrectly lowered #119352

bevin-hansson opened this issue Dec 10, 2024 · 7 comments
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@bevin-hansson
Copy link
Contributor

bevin-hansson commented Dec 10, 2024

See the following program: https://godbolt.org/z/YhdKov4x1

There's a number of inconsistencies in how this is emitted.

  • sizeof(_Complex _BitInt(144)) is 48, but the actual size of the declared variable is 64.

  • A struct with two _BitInts has a size of 48, both in sizeof and allocation.

  • The code in the set function is wrong:

    %arrayidx.imagp = getelementptr inbounds i8, ptr %p, i64 32
    store i144 1, ptr %p, align 8
    store i144 1, ptr %arrayidx.imagp, align 8

    If we allocate a memory area of size 48 and then perform these stores, we will write outside of the allocation (32 + 18 = 50).

    Compare this to the set2 function, which designates the imaginary member at offset 24.

@bevin-hansson bevin-hansson added the clang:codegen IR generation bugs: mangling, exceptions, etc. label Dec 10, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 10, 2024

@llvm/issue-subscribers-clang-codegen

Author: Bevin Hansson (bevin-hansson)

See the following program: https://godbolt.org/z/YhdKov4x1

There's a number of inconsistencies in how this is emitted.

  • sizeof(_Complex _BitInt(144)) is 48, but the actual size of the declared variable is 64.

  • A struct with two _BitInts has a size of 48, both in sizeof and allocation.

  • The code in the set function is wrong:

    %arrayidx.imagp = getelementptr inbounds i8, ptr %p, i64 32
    store i144 1, ptr %p, align 8
    store i144 1, ptr %arrayidx.imagp, align 8
    

    If we allocate a memory area of size 48 and then perform these stores, we will write outside of the allocation (32 + 18 = 50).

    Compare this to the set2 function, which designates the imaginary member at offset 24.

@tbaederr
Copy link
Contributor

@AaronBallman Are complex bitints even (supposed to be) a thing? Please say no

@Fznamznon
Copy link
Contributor

Fznamznon commented Dec 10, 2024

According to the standard IMO only floating complex types are allowed:

 There are three complex types, designated as float _Complex, double _Complex, and long double
_Complex. (Complex types are a conditional feature that implementations may not support; see
6.10.10.4.) The real floating and complex types are collectively called the floating types.

From what I can google, accepting integer _Complex types is a gcc extension https://gcc.gnu.org/onlinedocs/gcc/Complex.html . However gcc doesn't accept _Complex _BitInt. We probably need to decide if we want to support _Complex _BitInt as clang extension?
AFAIK Aaron is on vacation, cc @erichkeane

To codegen this correctly, approach taken in #91364 needs to be extended to take care of complex types.

@erichkeane
Copy link
Collaborator

According to the standard IMO only floating complex types are allowed:

 There are three complex types, designated as float _Complex, double _Complex, and long double
_Complex. (Complex types are a conditional feature that implementations may not support; see
6.10.10.4.) The real floating and complex types are collectively called the floating types.

From what I can google, accepting integer _Complex types is a gcc extension https://gcc.gnu.org/onlinedocs/gcc/Complex.html . However gcc doesn't accept _Complex _BitInt. We probably need to decide if we want to support _Complex _BitInt as clang extension? AFAIK Aaron is on vacation, cc @erichkeane

To codegen this correctly, approach taken in #91364 needs to be extended to take care of complex types.

I would think that unless we have a good reason to (and I don't really see one yet?) or someone who is volunteering to do the work, that just rejecting complex _BitInts is likely the best way forward. @Fznamznon : Could you perhaps prepare a patch to just reject _Complex _BitInt?

Fznamznon added a commit to Fznamznon/llvm-project that referenced this issue Dec 10, 2024
The C standard doesn't require support for these types and Codegen for
these types is incorrect ATM.
See llvm#119352
Fznamznon added a commit that referenced this issue Dec 12, 2024
The C standard doesn't require support for these types and Codegen for
these types is incorrect ATM.
See #119352
@bevin-hansson
Copy link
Contributor Author

Should this ticket be closed, then?

@erichkeane
Copy link
Collaborator

Fixed in #119402

@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed clang:codegen IR generation bugs: mangling, exceptions, etc. labels Dec 19, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 19, 2024

@llvm/issue-subscribers-clang-frontend

Author: Bevin Hansson (bevin-hansson)

See the following program: https://godbolt.org/z/YhdKov4x1

There's a number of inconsistencies in how this is emitted.

  • sizeof(_Complex _BitInt(144)) is 48, but the actual size of the declared variable is 64.

  • A struct with two _BitInts has a size of 48, both in sizeof and allocation.

  • The code in the set function is wrong:

    %arrayidx.imagp = getelementptr inbounds i8, ptr %p, i64 32
    store i144 1, ptr %p, align 8
    store i144 1, ptr %arrayidx.imagp, align 8

    If we allocate a memory area of size 48 and then perform these stores, we will write outside of the allocation (32 + 18 = 50).

    Compare this to the set2 function, which designates the imaginary member at offset 24.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
Development

No branches or pull requests

6 participants