This is an archive of the discontinued LLVM Phabricator instance.

[llvm][IPO] Add IR function attribute fine_grained_bitfields
AbandonedPublic

Authored by jmciver on Jul 12 2022, 12:26 AM.

Details

Summary

The IR function attribute fine_grained_bitfields is used to prevent the IPO
inlining of functions with different bit-field addressing schemes. Use of fine
grained and non fine grained bit-fields can result in data corruption. See the
following example:

// File A: compiled with -ffine-grained-bitfield-accesses
struct X {
  int a : 8;
  int b : 24;
};

void callee(struct X*);

int caller() {
  struct X x;
  x.a = 10;   // Variable a is directly stored to.
  callee(&x);
  return x.a;
}
// File B: compiled with -fno-fine-grained-bitfield-accesses
struct X {
  int a : 8;
  int b : 24;
};

void callee(struct X* x) {
  x->b = 10;  // Load occurs on struct object, followed by freeze,
                      // clear, set, and store sequence to assign b.
}

Because the caller uses fine-grained-bitfield-accesses, only the byte
associated with a is assigned and the value of b remains poison. The
callee does not have individual member variable addressing and thus loads the
full 32-bits (8-bits of value and 24-bits poison) resulting in a load of
poison. The proceeding freeze in the freeze, clear, set, and store sequence
will corrupt the already assigned value of a.

The IPO inlining issue was identified in D128501.

Diff Detail

Event Timeline

jmciver created this revision.Jul 12 2022, 12:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2022, 12:26 AM
jmciver edited the summary of this revision. (Show Details)Jul 12 2022, 12:33 AM
jmciver edited the summary of this revision. (Show Details)Jul 12 2022, 10:14 AM
jmciver published this revision for review.Jul 12 2022, 10:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2022, 10:18 AM
nikic edited reviewers, added: efriedma, nikic; removed: eli.friedman.Jul 12 2022, 1:12 PM
nikic added a subscriber: nikic.

I really don't like this. LLVM should not have any knowledge about "fine-grained bitfields". This also doesn't seem principled, in the sense that other IPO transforms (say, IPSCCP) could end up causing issues, not just inlining.

@nikic thanks for the feedback.

LLVM should not have any knowledge about "fine-grained bitfields".

I agree that strong division in tooling purpose is paramount. Ideally I would like to perform analysis during opt that would identify potential poison conflicts as to be agnostic to the particular pass (as there are four function inline pass types by my count). It appears post Clang CodeGen there is limited context for identifying if the code was generated in support of a bit-field initialization. Do you have any ideas?

This also doesn't seem principled, in the sense that other IPO transforms (say, IPSCCP) could end up causing issues, not just inlining.

I am aware of SCCP, but did not know of the interprocedural version. Thanks for bringing it to my attention. I'll take a look and discuss with my mentor @nlopes.

jdoerfert requested changes to this revision.Jul 13 2022, 7:35 AM

Is it legal to pass arguments that are compiled differently in the first place?
If we really want to prevent IPO + inlining we should have noinline and noipa, not something_specific_which_breaks_with_inlining_and_ipo.

This revision now requires changes to proceed.Jul 13 2022, 7:35 AM
jmciver abandoned this revision.Mar 23 2023, 9:42 AM

I am closing this ticket as we are working on alternative solutions to poison based load semantics.

Thanks to everyone for taking the time to discuss this patch and its limitations!