Hi Sean,
A change went in over the weekend to change the way GetElementPtrInst::Create works. Now, for pointer types, you need to manually specify the type of the pointee, as pointerType->getType() won’t work anymore. This broke the build, so a workaround was employed to simply pass nullptr for this argument. But this is likely to be incorrect, and instead we need the actual llvm::Type for the pointee. I’ve looked over the code, and I think that on line 2209 we can replace the nullptr with old_constant->getType(), and on line 2396 we can place the nullptr with value->getType().
What are your thoughts here?
Secondly, the change to nullptr does not seem to have caused any test failures. Is there any way we can add some tests for this? If this is not something that is testable through the public interface, I recently added gtest unit tests to the build. It’s not enabled in the Xcode build (but will be soon), but if you tell me how to set up a test, call the right function, and what outputs to expect, I can write it.
Hi Sean,
A change went in over the weekend to change the
way GetElementPtrInst::Create works. Now, for pointer types, you need to
manually specify the type of the pointee, as pointerType->getType() won't
work anymore. This broke the build, so a workaround was employed to simply
pass nullptr for this argument. But this is likely to be incorrect, and
instead we need the actual llvm::Type for the pointee. I've looked over
the code, and I *think* that on line 2209 we can replace the nullptr with
old_constant->getType(), and on line 2396 we can place the nullptr with
value->getType().
What are your thoughts here?
Secondly, the change to nullptr does not seem to have caused any test
failures.
This is expected - for now, nullptr is an acceptable value. The assertion I
have in GetElementPtrInst's ctor is:
assert(!PointeeType || PointeeType == getSourceElementType());
Eventually I'll need to remove the nullptr case once I believe I've flushed
out all the callers - this will verify that the callers are passing the
type that matches the pointee type of the pointer operand. (at this point
the current lldb code will assert-fail, if it is tested) You can try
removing the first part of that assert locally to see if the LLDB code path
is tested currently.
Once similar changes have been applied to all pointer-element-type-aware
uses of pointers in LLVM IR, I'll start trying to remove the ability to
query a pointer type for its element type.
So assuming the correct type is passed today, is that semantically equivalent to passing nullptr today? Because if not, then I would have expected a test to fail somewhere. Not because an assertion fired, but because something somewhere had the wrong idea about a type.
So assuming the correct type is passed today, is that semantically
equivalent to passing nullptr today?
Right - for now the new parameter is not used at all, except in the
assertion. So there's no way to get a change in behavior - either nullptr
is passed and nothing happens, or a type is passed and if the type matches,
nothing happens - if you get the type wrong, the assertion will fail.
This is a rather large migration effort, so I'm trying to take as small
steps as possible to ensure any issues are flushed out at each step before
changing the semantics out from underneath users.