Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[IR] Remove temporary const operator created in Value::getPointerAlignment()
AbandonedPublic

Authored by mlychkov on Feb 17 2020, 12:49 AM.

Details

Summary

New temporary constant operator may be created in getPointerAlignment() function.
This instance is not used by any instruction. However later it may lead to errors during
code processing as not expected.
Remove this constant after alignment calculation as no more required.

Diff Detail

Event Timeline

mlychkov created this revision.Feb 17 2020, 12:49 AM
mlychkov created this object with edit policy "Administrators".
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2020, 12:49 AM

I don't believe the question

...
Is this alignment accounting intended to be used for getelementptr instructions too?
If yes then may we cleanup extra user after we get TrailingZeros value?

Or is there a way to perform this computation without const_cast?

The problem is ConstantExpr::getPtrToInt. Do we striclty need that?

That is the workhorse here, we can't get around using it.

Without seeing the code where this is causing trouble i'm somewhat unsure
on how this is a problem, or how to test that the fix would actually fix it.
Perhaps, after computing TrailingZeros, we need to explicitly DCE CstInt?

That'll probably work as well. If CstInt has 0 users it can be removed.

Do we have an established precedent on this kind of issues?
I don't recall ever seeing such a problem before,
nor do i recall seeing manual cleaning up of tmp constantexprs.
I'm mainly wondering whether we actually have such guarantees that this violated (should not create new users)?

Sounds pretty odd to me too. From a quick grep, there's only a handful of destroyConstant() uses outside GlobalOpt, and those tend to deal with BlockAddress only (which is somewhat particular about uses).

was answered. Do we have this guarantee in the first place?

I don't believe the question

Sounds pretty odd to me too. From a quick grep, there's only a handful of destroyConstant() uses outside GlobalOpt, and those tend to deal with BlockAddress only (which is somewhat particular about uses).

was answered. Do we have this guarantee in the first place?

Maybe, we don't have it. But I thought it wasn't a good practice to change something in get-methods (except caches, maybe). If we can cleanup instances after some unexpected changes why not to do so?

If we can cleanup instances after some unexpected changes why not to do so?

The big problem here is that getPointerAlignment could be used in places where not all uses of a constant are actual "Use" instances. Then you have a really subtle use-after-free. (This could happen in places with maps on the side, like frontends or complex transforms).

If you really want to avoid creating extra constants, probably the right strategy is to avoid calling getPtrToInt in the first place. There aren't that many ways to construct a pointer that getPtrToInt can fold to a ConstantInt, anyway. (Off the top of my head, it would have to be null, an inttoptr, or a gep of one of those.)

If we can cleanup instances after some unexpected changes why not to do so?

The big problem here is that getPointerAlignment could be used in places where not all uses of a constant are actual "Use" instances. Then you have a really subtle use-after-free. (This could happen in places with maps on the side, like frontends or complex transforms).

If you really want to avoid creating extra constants, probably the right strategy is to avoid calling getPtrToInt in the first place. There aren't that many ways to construct a pointer that getPtrToInt can fold to a ConstantInt, anyway. (Off the top of my head, it would have to be null, an inttoptr, or a gep of one of those.)

I was hoping that would be a viable solution. Maybe matching null explicitly and otherwise just dealing with the constant that we have. IIRC, @lebedev.ri mentioned that would not work well but I let him explain.

If we can cleanup instances after some unexpected changes why not to do so?

The big problem here is that getPointerAlignment could be used in places where not all uses of a constant are actual "Use" instances. Then you have a really subtle use-after-free. (This could happen in places with maps on the side, like frontends or complex transforms).

If you really want to avoid creating extra constants, probably the right strategy is to avoid calling getPtrToInt in the first place. There aren't that many ways to construct a pointer that getPtrToInt can fold to a ConstantInt, anyway. (Off the top of my head, it would have to be null, an inttoptr, or a gep of one of those.)

I was hoping that would be a viable solution. Maybe matching null explicitly and otherwise just dealing with the constant that we have. IIRC, @lebedev.ri mentioned that would not work well but I let him explain.

I mean, sure, we can avoid using getPtrToInt and hardcode what it does here, but still, why?
Either the current state breaks guarantees and we should fix it,
or it does not and this roughly falls under the "c++ api is unstable" and whatever code
was relying on the unwritten guarantee should be relaxed not to rely on it.

When reading the following definition of method 'MaybeAlign Value::getPointerAlignment(const DataLayout &DL) const' most users assume that Value won't be modified by this method. If we don't guarantee it then it would be nice to remove const-qualifier and change name on something like 'accountPointerAlignment'. As renaming is less possible then a comment might be present that this method may change an IR.

The general rule for IR passes is that dangling constants may exist, and transformations are expected to correctly handle them (ignore or erase them, as appropriate). So creating them should be harmless across IR passes.

That's not really an excuse for IR queries that don't obviously involve creating constants to do so, though: it could be confusing within a pass. For example, if a pass is trying to iterate over a use list, and asking for the alignment modifies that use list.

mlychkov abandoned this revision.Feb 25 2020, 8:35 AM

I'm closing this review as current patch is not fixing the issue properly.