This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Handle enums
ClosedPublic

Authored by tbaeder on Sep 16 2022, 12:41 AM.

Diff Detail

Event Timeline

tbaeder created this revision.Sep 16 2022, 12:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 12:41 AM
tbaeder requested review of this revision.Sep 16 2022, 12:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2022, 12:41 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I can't come up with a way to get a reference to one of these, so the logic in the rest of the DeclRefExpr handling is a little MORE awkward looking now. I think I'm OK with this, but I want to give a few other folks a chance to look.

Also note, the CI test is from your test!

I can't come up with a way to get a reference to one of these, so the logic in the rest of the DeclRefExpr handling is a little MORE awkward looking now. I think I'm OK with this, but I want to give a few other folks a chance to look.

Ah right, I could to through that code path as well and not immediately return. I actually tested references but the AST expresses that through a MaterializeTemporaryExpr, which don't work yet.

Also note, the CI test is from your test!

Yup, that needs one of the other open patches.

shafik added inline comments.Sep 16 2022, 11:23 AM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
866

If I check out IntExprEvaluator::CheckReferenceDecl(...) it is checking the sign and width match between the expression and the EnumConstantDecl. I am guessing we need to do that here as well?

tbaeder added inline comments.Sep 16 2022, 12:27 PM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
866

I'm having a hard time trying to cause that code to trigger. e.g. in

enum Foo : unsigned int {
  A = -1,
};
static_assert(A == -1);

Both with the new and current interpreter, I get a`-Wc++11-narrowing` error by default, and the assertion works if I pass -Wno-c++11-narrowing.

tbaeder added inline comments.Sep 16 2022, 11:03 PM
clang/lib/AST/Interp/ByteCodeExprGen.cpp
866

Reading the code again, this seems to just compute a new value to not trigger assertions in Success(). Since those assertions don't exist in the new interpreter, the special casing shouldn't be needed.

Anything still missing here? (CI failure is because one of the other patches is missing)

tbaeder marked an inline comment as done.Sep 22 2022, 10:54 PM
shafik accepted this revision.Sep 23 2022, 1:20 PM

LGTM

clang/test/AST/Interp/enums.cpp
26

Maybe some edge case values for enumerators like __INT_MAX__ *2U +1U (UINT_MAX)

and

enum E { // warning: enumeration values exceed range of largest integer [-Wenum-too-large]
  E1 = -__LONG_MAX__ -1L,
  E2 = __LONG_MAX__ *2UL+1UL 
};
This revision is now accepted and ready to land.Sep 23 2022, 1:20 PM
tbaeder updated this revision to Diff 462826.Sep 26 2022, 12:02 AM
tbaeder marked an inline comment as done.
This revision was landed with ongoing or failed builds.Sep 29 2022, 3:51 AM
This revision was automatically updated to reflect the committed changes.
tbaeder added inline comments.Sep 29 2022, 4:37 AM
clang/test/AST/Interp/enums.cpp
26

Hm, looks like that test broke one of the windows builders: https://lab.llvm.org/buildbot/#/builders/123/builds/13424 - are enums larger by default on Windows? What do you suggest to fix the test?

aaron.ballman added inline comments.Sep 29 2022, 5:02 AM
clang/test/AST/Interp/enums.cpp
26

If we're trying to be compatible with MSVC, we use their rules for picking the underlying type of an enumeration is which not fixed. One way to handle this is to add more RUN lines with explicit triples, but I think we lose too much interesting coverage that way. I'd probably use a #ifndef _MSC_VER block to control the expected diagnostics with a comment as to why the diagnostic is not expected on Windows.

tbaeder added inline comments.Sep 29 2022, 6:09 AM
clang/test/AST/Interp/enums.cpp
26

The second builder that broke was a hexagon builder: https://lab.llvm.org/buildbot/#/builders/38/builds/6231 - this would still fail with the _MSC_VER change, right?

aaron.ballman added inline comments.Sep 29 2022, 6:21 AM
clang/test/AST/Interp/enums.cpp
26

Nope, drat.

I would recommend breaking that specific test out into a separate file where we can add various RUN lines with triples, and the rest of the test can remain targetless.

tbaeder added inline comments.Sep 29 2022, 6:30 AM
clang/test/AST/Interp/enums.cpp
26

Shame, but alright. So, what triples should I test? :)

aaron.ballman added inline comments.Sep 29 2022, 6:34 AM
clang/test/AST/Interp/enums.cpp
26

The ones from the failing bots, and I'd say maybe x86_64-pc-linux and i686-pc-linux would be reasonable.

tbaeder added inline comments.Sep 29 2022, 8:58 AM
clang/test/AST/Interp/enums.cpp
26

Heh, the warning doesn't happen with the i686 triple either (with the old interpreter). Is that expected?

aaron.ballman added inline comments.Sep 29 2022, 12:10 PM
clang/test/AST/Interp/enums.cpp
26

Yeah, that sounds about like what I'd expect.