Page MenuHomePhabricator

Inline asm constraints: allow ICE-like pointers for the "n" constraint (PR40890)
ClosedPublic

Authored by hans on Mar 1 2019, 5:27 AM.

Details

Summary

Apparently GCC allows this, and there's code relying on it (see bug, which is a release blocker for llvm 8).

The idea is to allow expression that would have been allowed if they were cast to int. So I based the code on how such a cast would be done (the CK_PointerToIntegral case in IntExprEvaluator::VisitCastExpr()).

I'm unfamiliar with this code, especially the LValue variant of APValue, so please take a careful look.

Diff Detail

Repository
rC Clang

Event Timeline

hans created this revision.Mar 1 2019, 5:27 AM
void added a comment.Mar 1 2019, 11:25 AM

My opinion doesn't carry as much weight as others who are more familiar with the front-end code, but LGTM.

One question, the code you added looks similar. Is there a way to extrapolate it into its own function? Maybe yet another EvaluateAs* method?

clang/lib/Sema/SemaStmtAsm.cpp
389 ↗(On Diff #188887)

s/allows/allow/

efriedma added inline comments.Mar 1 2019, 12:07 PM
clang/lib/CodeGen/CGStmt.cpp
1852 ↗(On Diff #188887)

This always returns an APSInt with width 64; is that really right? I guess it might not really matter given that it's only going to be used as an immediate constant anyway, but it seems weird.

clang/lib/Sema/SemaStmtAsm.cpp
394 ↗(On Diff #188887)

APValue::isNullPointer() asserts that the value is an LValue; do you need to check for that explicitly here?

399 ↗(On Diff #188887)

I think it makes sense to add a method to APValue specifically to do the conversion from LValue to an APSInt, whether or not isNullPointer() is true, and use it both here and in IntExprEvaluator::VisitCastExpr in lib/AST/ExprConstant.cpp. The logic is sort of subtle (and I'm not completely sure it's right for targets where null is not zero, but you shouldn't try to fix that here).

joerg added a subscriber: joerg.Mar 2 2019, 12:44 PM

Can you include a patch for something like (int *)0xdeadbeeeeeef on amd64? That's a valid value for "n", but clearly too large for int. Thanks for looking at this, it is one of the two large remaining show stoppers for the asm constraint check.

hans marked 6 inline comments as done.Mar 4 2019, 3:13 AM

Can you include a patch for something like (int *)0xdeadbeeeeeef on amd64? That's a valid value for "n", but clearly too large for int. Thanks for looking at this, it is one of the two large remaining show stoppers for the asm constraint check.

You mean to check that we don't truncate or otherwise choke on it? Sure, I'll add it.

clang/lib/CodeGen/CGStmt.cpp
1852 ↗(On Diff #188887)

I agree it seems a little strange, but I think in practice it's correct. EVResult.Val.getLValueOffset().getQuantity() returns an int64_t, so we're not losing any data.

The code that I lifted this from, is using the bitwidth of the casted-to integer type for the result. But it's still only got maximum 64 bits since the source, getLValueOffset().getQuantity(), is the same.

clang/lib/Sema/SemaStmtAsm.cpp
389 ↗(On Diff #188887)

Done.

394 ↗(On Diff #188887)

Yes I do. Thanks!

399 ↗(On Diff #188887)

I agree (and this was also Bill's suggestion above) that it would be nice to have a utility method for this.

I'm not sure adding one to APValue would work for IntExprEvaluator::VisitCastExpr though, since that code is actually using its own LValue class, not an APValue until it's time to return a result.

I frankly also doesn't fully understand what that code is doing. If the LValue has a base value, it seems to just take that as result and ignore any offset? This is unknown territory to me, but the way I read it, if there's an lvalue base, the expression isn't going to come out as an integer constant. I think.

About null pointers, I'm calling getTargetNullPointerValue() so I think that should be okay, no?

hans updated this revision to Diff 189129.Mar 4 2019, 3:14 AM
hans marked 2 inline comments as done.

Address comments.

hans added a comment.Mar 4 2019, 3:14 AM

Can you include a patch for something like (int *)0xdeadbeeeeeef on amd64? That's a valid value for "n", but clearly too large for int. Thanks for looking at this, it is one of the two large remaining show stoppers for the asm constraint check.

What's the other show stopper? Is that also something that regressed from the previous release since the "n" constraint got stricter?

joerg added a comment.Mar 4 2019, 3:37 PM

The other problem is that we don't use the CFG machinery to prune dead branches. Consider the x86 in/out instructions: one variant takes an immediate, the other a register. The classic way to deal with that is something like

static inline void outl(unsigned port, uint32_t value)
{
  if (__builtin_constant_p(port) && port < 0x100) {
    __asm volatile("outl %0,%w1" : : "a" (data), "id" (port));
  } else {
   __asm volatile("outl %0,%w1" : : "a" (data), "d" (port));
  }
}

This fails with the new asm constraint checks, since the dead branch is never pruned. For other architectures it makes an even greater difference. The main reason it is a show stopper: there is no sane workaround that doesn't regress code quality.

This fails with the new asm constraint checks, since the dead branch is never pruned.

As far as I know, we didn't touch "i", only "n". Is there a bug filed for the issue you're describing?

clang/lib/CodeGen/CGStmt.cpp
1852 ↗(On Diff #188887)

The concern isn't that we would lose data. I'm more concerned the backend might not be prepared for a value of the "wrong" width.

clang/lib/Sema/SemaStmtAsm.cpp
399 ↗(On Diff #188887)

Oh, I didn't realize IntExprEvaluator::VisitCastExpr wasn't using the same class to represent the value; that makes it harder to usefully refactor. But still, it would be good to reduce the duplicated code between here and CodeGen.

If the LValue has a base value, it seems to just take that as result and ignore any offset?

If there's a base value, it returns the whole LValue, including the base and offset.

I'm calling getTargetNullPointerValue() so I think that should be okay

The issue would be the case where you have a null pointer with an offset, like the case in the bug. It's sort of inconsistent if null==-1, but null+1==1. But it's not something we handle consistently elsewhere, anyway, so I guess we can ignore it for now.

joerg added a comment.Mar 5 2019, 5:48 AM

Well, that was a sample to illustrate the point. A full working (and now failing) example is:

static inline void outl(unsigned port, unsigned data) {
  if (__builtin_constant_p(port) && port < 0x100) {
    __asm volatile("outl %0,%w1" : : "a"(data), "n"(port));
  } else {
    __asm volatile("outl %0,%w1" : : "a"(data), "d"(port));
  }
}

void f(unsigned port) { outl(1, 1); }
hans marked 2 inline comments as done.Mar 5 2019, 7:31 AM
hans added inline comments.
clang/lib/CodeGen/CGStmt.cpp
1852 ↗(On Diff #188887)

Oh, I see. I'll change the code to use ASTContext::MakeIntValue with the source type. Apparently this works even if the source type is a pointer type; I guess that yields an integer of the same width. I think maybe that's the best we can do?

clang/lib/Sema/SemaStmtAsm.cpp
399 ↗(On Diff #188887)

Ah, the null pointer issue is interesting, but like you say we don't seem to handle this in the cast code I was inspired by here either.

hans updated this revision to Diff 189319.Mar 5 2019, 7:32 AM

Extract code to a new method in APValue.

Eli, what do you think about something like this? Suggestions for better name welcome :-)

This revision is now accepted and ready to land.Mar 5 2019, 11:38 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 2:25 AM