[PATCH] D155023: Don't assert on a non-pointer value being used for a "p" inline asm constraint.
Amara Emerson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 12 20:59:45 PDT 2023
aemerson added a comment.
In D155023#4495929 <https://reviews.llvm.org/D155023#4495929>, @pengfei wrote:
> My concern is removing assert don't solve the real problem. I think there are two problems here:
>
> 1. Compiler shouldn't assert for invalid semantics. That's true. We could solve it by detecting such invalid semantics during sema checking.
> 2. Application relies on invalid semantics. This is my main concern. Besides, I took a look at GCC and find it report error for it https://godbolt.org/z/zPWhsY5aj
In D155023#4495929 <https://reviews.llvm.org/D155023#4495929>, @pengfei wrote:
> My concern is removing assert don't solve the real problem. I think there are two problems here:
>
> 1. Compiler shouldn't assert for invalid semantics. That's true. We could solve it by detecting such invalid semantics during sema checking.
> 2. Application relies on invalid semantics. This is my main concern. Besides, I took a look at GCC and find it report error for it https://godbolt.org/z/zPWhsY5aj
Sorry the inline assembly in the test isn't actually valid for ud1l, here's an example showing 'p' being used with an integer literal: https://godbolt.org/z/sPqqj5q8M
void *test()
{
void *ret;
asm ("movq %1, %0" : "=r" (ret) : "p" (256));
return ret;
}
You'll find that gcc happily accepts this. For non-standardized features like this, the semantics are defined by what the implementations *do*. If GCC has always rejected this as an error, I'm happy to do the same. But GCC doesn't, so we should match them instead of breaking existing users' code (that includes throwing an error in Sema).
Now if you are convinced that this deletion of the assert is incorrect, then we can discuss a better fix. The other option is that we entirely back out the original change D133914 <https://reviews.llvm.org/D133914> that introduced this regression.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D155023/new/
https://reviews.llvm.org/D155023
More information about the llvm-commits
mailing list