This is an archive of the discontinued LLVM Phabricator instance.

[Clang][Codegen] Truncate initializers of union bitfield members
ClosedPublic

Authored by tmatheson on Dec 11 2020, 3:30 AM.

Details

Summary

If an initial value is given for a bitfield that does not fit in the
bitfield, the value should be truncated. Constant folding for
expressions did not account for this truncation in the case of union
member functions, despite a warning being emitted. In some contexts,
evaluation of expressions was not enabled unless C++11, ROPI or RWPI
was enabled.

Diff Detail

Event Timeline

tmatheson requested review of this revision.Dec 11 2020, 3:30 AM
tmatheson created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2020, 3:30 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
joechrisellis added inline comments.Jan 13 2021, 3:46 AM
clang/lib/AST/ExprConstant.cpp
9803–9804

nit: I would prefer this:

if (Field->isBitField() && truncateBitfieldValue(Info, InitExpr, Result.getUnionValue(), Field))
    return true;

return EvaluateInPlace(Result.getUnionValue(), Info, Subobject, InitExpr);

It feels more in-line with the rest of the function. But it is okay if you want to ignore this too. 😄

clang/test/CodeGenCXX/bitfield-layout.cpp
88–95

I'd like to see some more tests that check the truncation behaviour. My understanding is that this is trucating to -1 because of two's complement? How about something like:

int test_trunc() {
    union {
        int i : 4;
    } U = {80};
    return U.i;
    // CHECK: ret i32 0
}

Am I understanding the behaviour correctly?

Some comments about what is actually happening on the bit-level to get this result would also be nice.

rsmith added inline comments.Jan 13 2021, 10:54 AM
clang/lib/AST/ExprConstant.cpp
9801–9804

Shouldn't this be && not ||? These functions return true if they succeed (unlike the convention in Sema where true means an error diagnostic was produced).

RKSimon resigned from this revision.Jan 14 2021, 2:05 AM
tmatheson updated this revision to Diff 317306.Jan 18 2021, 2:36 AM

Added more test cases and explanatory comments

tmatheson updated this revision to Diff 317309.Jan 18 2021, 2:45 AM

Add test RUN line that checks C++11 behaviour

tmatheson updated this revision to Diff 317340.Jan 18 2021, 5:58 AM

Make unions in test cases const, and clarify logic

tmatheson marked 3 inline comments as done.Jan 18 2021, 6:03 AM
tmatheson added inline comments.
clang/lib/AST/ExprConstant.cpp
9801–9804

You are correct. I have updated the logic to be simpler to follow as well.

clang/test/CodeGenCXX/bitfield-layout.cpp
88–95

Yes that is correct. I have added some new test cases and comments to make it clear what they are testing for.

tmatheson marked 2 inline comments as done.Jan 25 2021, 7:17 AM

Ping

pratlucas accepted this revision.Jan 27 2021, 8:31 AM

The truncate conditions look a lot better and the test coverage seems reasonable now.
LGTM.

This revision is now accepted and ready to land.Jan 27 2021, 8:31 AM
This revision was landed with ongoing or failed builds.Jan 28 2021, 1:21 AM
This revision was automatically updated to reflect the committed changes.