This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Implement remaining strcmp builtins
Needs ReviewPublic

Authored by tbaeder on Jul 25 2023, 12:05 AM.

Diff Detail

Event Timeline

tbaeder created this revision.Jul 25 2023, 12:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2023, 12:05 AM
tbaeder requested review of this revision.Jul 25 2023, 12:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2023, 12:05 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.Jul 25 2023, 6:34 AM
clang/lib/AST/Interp/InterpBuiltin.cpp
88–92

Allowing char8_t seems wrong to me -- https://godbolt.org/z/zK74hMP7q

147–149

Let's either implement support for them in this patch or exclude the wchar_t changes entirely from the patch?

clang/test/AST/Interp/builtin-functions.cpp
225–229

Why are these in a #if 0 block?

shafik added inline comments.Jul 26 2023, 1:41 PM
clang/test/AST/Interp/builtin-functions.cpp
17

How about -1 size as well.

cor3ntin added inline comments.Jul 27 2023, 12:46 AM
clang/lib/AST/Interp/InterpBuiltin.cpp
88–92

Just agreeing with Aaron there.
I don't have a problem with making __builtin_strcmp work with any charN_t but in this case we need to make sure both parameters are of the same element type.
Your implementation seems to allow __builtin_strcmp(u8"", "") which is comparing very different things. ie __builtin_strcmp(u8"a", "z") could be true, which is really not something we want to support :)

There are still unaddressed comment here. It would be nice to complete this before phab shuts down

I have a local patch that implements these builtins by doing a bitcast to a buffer first. There is a comment in ExprConstant.cpp that talks about the same thing. That path probably makes more sense? I'd abandon this review then.