[PATCH] D105710: [OpaquePointers][ThreadSanitizer] Cleanup calls to PointerType::getElementType()

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

In D105710#2874527 <https://reviews.llvm.org/D105710#2874527>, @dblaikie wrote:

> In D105710#2872325 <https://reviews.llvm.org/D105710#2872325>, @dblaikie wrote:
>
>> In D105710#2872101 <https://reviews.llvm.org/D105710#2872101>, @aeubanks wrote:
>>
>>> I still think it's not worth it to find some test that covers this case.
>>>
>>> Getting rid of calls to `PointerType::getElementType()` in it of itself is a goal. If we had done this before the opaque pointer type were introduced we wouldn't need tests and it would have gone in as an NFC. As long as we're not adding functionality I don't think tests are super valuable.
>>>
>>> IMO we should try to remove as many calls to get pointee types as possible before we start running check-llvm with --force-opaque-pointers and combing through crashes. We'll get the same test coverage either way in the end.
>>
>> I feel pretty strongly against (on principle - if it's testable, it should be tested - the change isn't NFC now that we have opaque pointer types), but won't hold this up on that.
>>
>> Might be worth an assert in getMemoryAccessFuncIndex that we've tracked the pointer element type correctly (assert isOpaqueOrPointeeTypeMatches) to demonstrate that the type hasn't changed, we're just tracking it differently.
>>
>> [Though, OK, I'm a bit hung up on this: Would it be terribly expensive to take an existing test and add a new RUN line to run it with opaque pointers & then it'd be clear that these changes were adequate/necessary/correct to make this code work correctly with opaque pointers]
>
> Any further thoughts on this ^ ? Adding testing in other cases has been possible/practical, such as D105398 <https://reviews.llvm.org/D105398>

I agree that D105398 <https://reviews.llvm.org/D105398> requires testing because it's actually doing something different depending on whether or not we're looking at opaque pointers, so we need to test the new branch being added.

IMO there are a couple ways to fix issues that pop up due to opaque pointers. One is to run check-llvm with --force-opaque-pointers on and look at/fix crashes. This is probably the most "proper" way to go about this. But I think a faster way is to first go around cleaning up calls to `getElementType()`, since a vast majority of those will be issues that will pop up later. Even if not all of these are real issues, it's still a quick way to clean up a significant amount of the crashes we'd see later. Either way, in the end we'll be running check-llvm with --force-opaque-pointers and get the same test coverage. But cleaning up with easy fixes to remove `getElementType()` makes things go faster to begin with.

Perhaps the more involved fixes that take a while to fix should have tests. But for easy/obvious things I'd like to get them out of the way quickly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105710/new/

https://reviews.llvm.org/D105710