This is an archive of the discontinued LLVM Phabricator instance.

[clang][Interp] Fix getIndex() for composite array elements
ClosedPublic

Authored by tbaeder on Aug 16 2023, 4:02 AM.

Diff Detail

Event Timeline

tbaeder created this revision.Aug 16 2023, 4:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2023, 4:02 AM
tbaeder requested review of this revision.Aug 16 2023, 4:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2023, 4:02 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tbaeder updated this revision to Diff 550696.Aug 16 2023, 4:23 AM
cor3ntin added inline comments.Aug 16 2023, 5:19 AM
clang/lib/AST/Interp/Context.h
90

This should return a const ref

clang/lib/AST/Interp/Pointer.h
220

WS only change

340

Can you be a bit more verbose here? I can't figure out what is happening from the comment!

tbaeder updated this revision to Diff 557097.Sep 19 2023, 11:54 PM
tbaeder marked an inline comment as done.
tbaeder added inline comments.Sep 20 2023, 1:31 AM
clang/lib/AST/Interp/Context.h
90

Things like getGlobal() aren't const so that doesn't work.

clang/lib/AST/Interp/Pointer.h
340

For primitive arrays, the elements don't have inline descriptors:

                     Offset
                     │
                     ▼
┌────────┬───┬───┬───┬───┬───┬─────┐
│InitMap │ 1 │ 2 │ 3 │ 4 │ 5 │ ... │
└────────┴───┴───┴───┴───┴───┴─────┘
▲
│
Base

If the array elements are composite elements themselves however, we can have a pointer that refers to the array element:

               Offset
               │
               ▼
┌──────────┬───┬──────────┬───┐
│InlineDesc│'a'│InlineDesc│'b'│
└──────────┴───┴──────────┴───┘
▲
│
Base

(so things like isArrayElement() return true) as well as a pointer that _only_ refers to the composite element (after a call to narrow():

               Offset
               │
               ▼
┌──────────┬───┬──────────┬───┐
│InlineDesc│'a'│InlineDesc│'b'│
└──────────┴───┴──────────┴───┘
               ▲
               │
               Base

For the latter, the expected value of getIndex() is 0, since it is (as far as the pointer knows) _not_ an array element.

aaron.ballman added inline comments.Sep 28 2023, 9:10 AM
clang/lib/AST/Interp/Context.h
90

If this is only needed for testing, can we make it a private function and use friendship to expose it to just what needs it? (Esp because it's got some odd const-correctness properties to it.)

clang/lib/AST/Interp/Pointer.h
81–88

Same here -- can these be private and friended?

tbaeder added inline comments.Sep 30 2023, 10:34 PM
clang/lib/AST/Interp/Pointer.h
81–88

Don't you need a class to friend something? I only have the TEST(...) function in the unit test, so I can't do that, right?

aaron.ballman added inline comments.Oct 2 2023, 9:59 AM
clang/lib/AST/Interp/Pointer.h
81–88
tbaeder added inline comments.Oct 2 2023, 9:48 PM
clang/lib/AST/Interp/Pointer.h
81–88

Is this something we should be doing? There's nothing else in clang using FRIEND_TEST and only stuff in Testing/ includes gtest.h.

aaron.ballman added inline comments.Oct 4 2023, 7:05 AM
clang/lib/AST/Interp/Pointer.h
81–88

It's a tradeoff as to whether we want to expose private implementation details as part of a public interface just to enable unit testing, or whether we want to sprinkle unit testing annotations around the private implementation details just to enable unit testing. Personally, I prefer having cleaner public interfaces; otherwise we end up with people using the implementation details of a class and it's harder to refactor in the future. I'm not certain how others feel, though.

tbaeder added inline comments.Oct 9 2023, 6:34 AM
clang/lib/AST/Interp/Pointer.h
81–88

I think FRIEND_TEST just doesn't work because I can't import a gtest header from the clangAST library.

What I can do is just wrap those things in a #ifdef IN_UNITTEST and define than before including those headers (all of this code is in headers right now) in the unittest/AST/Interp/Descriptor.cpp.

aaron.ballman added inline comments.Oct 9 2023, 8:26 AM
clang/lib/AST/Interp/Pointer.h
81–88

Okay, that seems like a good compromise to me. Later, we might want to figure out if we can hoist the macro logic up to a higher level so that other interfaces can use the same pattern. (e.g., maybe we want LLVM_UNIT_TEST_INTERFACE macro or some such?)

tbaeder updated this revision to Diff 557656.Oct 9 2023, 9:32 PM
This revision is now accepted and ready to land.Oct 11 2023, 10:09 AM
This revision was landed with ongoing or failed builds.Oct 12 2023, 4:09 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Oct 12 2023, 5:34 AM
thakis added inline comments.
clang/unittests/AST/Interp/CMakeLists.txt
2

Why is this in a separate executable instead of in ASTTests? So it can have fewer deps?

tbaeder added inline comments.Oct 12 2023, 6:15 AM
clang/unittests/AST/Interp/CMakeLists.txt
2

Mostly because running ASTTests takes very long