llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.08k stars 11.59k forks source link

clang integer complex divide algorithm is wrong #104738

Open efriedma-quic opened 1 month ago

efriedma-quic commented 1 month ago

Consider the following (taken from #104666):

constexpr auto GH55390 = 1 / 65536j;

Current clang prints a divide by zero message... but that's nonsense: the denominator isn't zero. We should be able to evaluate this without overflow.

llvmbot commented 1 month ago

@llvm/issue-subscribers-clang-frontend

Author: Eli Friedman (efriedma-quic)

Consider the following (taken from #104666): ``` constexpr auto GH55390 = 1 / 65536j; ``` Current clang prints a divide by zero message... but that's nonsense: the denominator isn't zero. We should be able to evaluate this without overflow.
llvmbot commented 1 month ago

@llvm/issue-subscribers-clang-codegen

Author: Eli Friedman (efriedma-quic)

Consider the following (taken from #104666): ``` constexpr auto GH55390 = 1 / 65536j; ``` Current clang prints a divide by zero message... but that's nonsense: the denominator isn't zero. We should be able to evaluate this without overflow.
c8ef commented 1 month ago

Hi, I would like to continue working on this issue. I am curious if the overflow issue is due to the APSInt field not being wide enough to store the product of the multiplication.

pinskia commented 1 month ago

Note GCC has talked about deprecated and removing complex integer support a few times. See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=2995 which references complex integer division issues. and https://gcc.gnu.org/legacy-ml/gcc/2001-11/msg00790.html

I will try to bring up about deprecating them again on the GCC list tomorrow.

pinskia commented 1 month ago

I will try to bring up about deprecating them again on the GCC list tomorrow.

https://gcc.gnu.org/pipermail/gcc/2024-August/244613.html

AaronBallman commented 1 month ago

The issue happens with either the new or the current constexpr interpreter, though the new interpreter fails to say why the division isn't a constant expression.

https://godbolt.org/z/KajqWKq9z

AaronBallman commented 1 month ago

I will try to bring up about deprecating them again on the GCC list tomorrow.

https://gcc.gnu.org/pipermail/gcc/2024-August/244613.html

Some supporting evidence for just how disruptive this will be: https://sourcegraph.com/search?q=context:global+%5E_Complex%5Cs%2B%28int%7Cshort%7Cunsigned%7Csigned%29+-file:.*gcc.*+-file:.*clang.*+-file:.*test.*+file:.*%5C.%28c%7CC%7Ccpp%7Ccxx%7Ch%29&patternType=regexp&case=yes&sm=0

(lol, I suspect we'll be okay with deprecation as well.)

pinskia commented 1 month ago

Some supporting evidence for just how disruptive this will be: https://sourcegraph.com/search?q=context:global+%5E_Complex%5Cs%2B%28int%7Cshort%7Cunsigned%7Csigned%29+-file:.*gcc.*+-file:.*clang.*+-file:.*test.*+file:.*%5C.%28c%7CC%7Ccpp%7Ccxx%7Ch%29&patternType=regexp&case=yes&sm=0

Note your regex can be expanded slightly to include white spaces at the begining of the line add /s after the ^). Though there are 2 uses that I could find: libobjc2 LTE code (e.g. https://sourcegraph.com/github.com/wineslab/colosseum-scope/-/blob/radio_code/srsLTE/lib/src/phy/io/filesink.c)

The libobjc2 code becomes unused as the encoding becomes unused for new code, and could just become an array of the integer types.

The LTE code can be just converted into using an array and/or 2 memcpy.