[PATCH] D49403: More aggressively complete RecordTypes with Function Pointers

[Andy Kaylor via Phabricator via cfe-commits]

andrew.w.kaylor added a comment.

I hope I'm not coming across as too argumentative here. I don't really have strong feelings about this function pointer type patch and ultimately I see that you are right, but the objections you are raising here would equally apply to what I'm working on with the IR linker and if I find a way to fix that, I'll care a bit more about that case. (If you'd like a preview, here's the bug I'm trying to fix: https://bugs.llvm.org/show_bug.cgi?id=38408)

You say "there is no semantic meaning for pointer types in LLVM IR" and that's fine, but there is a function PointerType::getElementType() and if I modify that function to always return the i8 type it will break a lot of things. So while there may be some sense in which the the pointer type cannot be the "correct" type, there is most definitely a sense in which it can be "incorrect" even if that sense isn't the strict semantic sense. I haven't looked at all the uses of  PointerType::getElementType() [or the related Type::getPointerElementType()]. I know a lot of them are just tests and assertions. Others are trivially walking through something that they know to be true by other means. A few seem (at least on first glance) to actually be doing something with it. For instance, llvm::getPointerStride() contains this line of code: "int64_t Size = DL.getTypeAllocSize(PtrTy->getElementType());"

I'm not arguing against opaque pointer types. I just feel like we're probably at least a couple of years away from having that. I'm also not arguing for robust and exhaustive correction of all cases where pointer types are not currently "working" in the sense that I am describing in my prior comments. I'm just saying that if someone runs into a specific case that is causing them problems and they submit a fix for that case, maybe we should let it through unless we have more of a reason than strict semantics rendering it unimportant.


https://reviews.llvm.org/D49403