Page MenuHomePhabricator

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

Authored by aeubanks on Jul 9 2021, 9:26 AM.

Diff Detail

Event Timeline

aeubanks created this revision.Jul 9 2021, 9:26 AM
aeubanks requested review of this revision.Jul 9 2021, 9:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2021, 9:26 AM
aeubanks added a reviewer: Restricted Project.Jul 9 2021, 9:26 AM
aeubanks retitled this revision from [ThreadSanitizer] Cleanup calls to PointerType::getElementType() to [OpaquePointers][ThreadSanitizer] Cleanup calls to PointerType::getElementType().Jul 9 2021, 9:34 AM

Is this able to be tested with an opaque pointer IR input to run this pass?

Probably, but I don't think it's worth the time to come up with a test case for every opaque pointer change that doesn't affect existing typed pointer IR. There are a lot of calls in many places throughout LLVM to PointerType::getElementType()/Type::getPointerElementType(). The important thing is that this is NFC for existing typed pointer IR and we're removing calls to those functions.

Probably, but I don't think it's worth the time to come up with a test case for every opaque pointer change that doesn't affect existing typed pointer IR. There are a lot of calls in many places throughout LLVM to PointerType::getElementType()/Type::getPointerElementType(). The important thing is that this is NFC for existing typed pointer IR and we're removing calls to those functions.

I think it's pretty important that we test the changes we're making to demonstrate/ensure they do what we intend.

nikic added a subscriber: nikic.Jul 9 2021, 12:24 PM

Probably, but I don't think it's worth the time to come up with a test case for every opaque pointer change that doesn't affect existing typed pointer IR. There are a lot of calls in many places throughout LLVM to PointerType::getElementType()/Type::getPointerElementType(). The important thing is that this is NFC for existing typed pointer IR and we're removing calls to those functions.

I think it's pretty important that we test the changes we're making to demonstrate/ensure they do what we intend.

My 2c here is that we mainly need to test cases where we're doing something non-obvious for opaque pointers. E.g. D105398 uses a different code path for opaque pointers, so that's worth testing. Here we use the same code and only adjust the APIs used.

Main problem with testing these is that making the code change is easy without any domain knowledge about the pass, while writing tests for it requires you to actually figure out what it is supposed to be doing and how you can trigger the relevant code paths.

llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp
617–618

Could you getLoadStorePointerOperand() here.

Probably, but I don't think it's worth the time to come up with a test case for every opaque pointer change that doesn't affect existing typed pointer IR. There are a lot of calls in many places throughout LLVM to PointerType::getElementType()/Type::getPointerElementType(). The important thing is that this is NFC for existing typed pointer IR and we're removing calls to those functions.

I think it's pretty important that we test the changes we're making to demonstrate/ensure they do what we intend.

My 2c here is that we mainly need to test cases where we're doing something non-obvious for opaque pointers. E.g. D105398 uses a different code path for opaque pointers, so that's worth testing. Here we use the same code and only adjust the APIs used.

But we did so for a reason - to enable some functionality that wasn't enabled before - and we haven't tested that we successfully did what we intended to do.

Main problem with testing these is that making the code change is easy without any domain knowledge about the pass, while writing tests for it requires you to actually figure out what it is supposed to be doing and how you can trigger the relevant code paths.

Yeah, it's not cheap, no doubt. My usual approach/suggestion would be to put an "assert(false)" in the code being changed, run check-llvm and see which tests fail, pick a fairly simple one and try the same test with opaque pointers.

Alternatively, maybe starting from the tests rather than the code would be good at this point - running chunks of the test suite with opaque pointers forced on, seeing what fails and fixing them - then it's easy to know what testing to add because it started with a failing test case.

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.

dblaikie accepted this revision.Jul 12 2021, 2:42 PM

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]

This revision is now accepted and ready to land.Jul 12 2021, 2:42 PM
aeubanks updated this revision to Diff 358133.Jul 12 2021, 8:46 PM

add assert

This revision was landed with ongoing or failed builds.Jul 12 2021, 8:46 PM
This revision was automatically updated to reflect the committed changes.

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

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

I agree that 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.

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

I agree that 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.

I thought of an alternative, though may require differently more work: What if we had a green/red list of test directories that pass with --force-opaque-pointers and a buildbot that ran in this configuration, but only alerted for whole directories that were meant to be green that contained failing tests? Then we could update the green list when fixes are made that make a difference, ensuring the fixes are correct/effective, validated, and don't regress?

It'd maybe mean part-way between the pure source based solution, and the test based solution - because you could still identify issues by pure source inspection, but I think then expand it to "what else in this transform/tool/use case isn't able to handle opaque pointers" until there's at least enough to make some test change to green?