flag non-portable bitfields

[WalterBright]

Discussed at the last DLF mtg.

The bitfield code is not amenable to flagging nonportable layouts, so I'm trying to refactor it to make it more tractable.

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#20840"

[rikkimax]

This should probably mention other ABI's like Windows i.e.

Suggested change

	error(bfd.loc, "bitfield layout not portable among supported C compilers, wrap with `extern (C)` or `extern (C++)`");
	error(bfd.loc, "bitfield layout not portable among supported C compilers, wrap with extern (X) where X is C, C++, Windows, or System");

[ibuclaw]

This is target specific though. Each compiler back-end makes its own decision about this.

[ibuclaw]

For example, in GCC, back-ends set a PCC_BITFIELD_TYPE_MATTERS macro to notify us that type affects alignment. Then TARGET_ALIGN_ANON_BITFIELD if anonymous bitfields affect alignment as well.

Taking this away would likely regress data layouts on those back-ends.

[WalterBright]

What matters is which ones are actually different.

[WalterBright]

I have run into a significant problem. The idea is to use extern (C): at the start of the source file to make all the bitfields not issue an error for a non-portable layout. The problem is that doing the layout of struct fields happens before the semantic phase, when the linkage is not known. The extern (C): would have to be inside the struct declaration to be noticed by the layout code.

So we're back to the ugly scheme I objected to (this scheme was sold as just putting the extern (C): at the start of the module).

struct S {
    extern (C) { int a:3, b:4; }  // barf
}

We could rerun all the struct layouts during the semantic phase, but that would be more complexity and runtime cost for what is an issue that is not significant anyway.

[rikkimax]

Could we call a function as part of the glue code to detect this?

[WalterBright]

It's a good question. The problem is the absurd complexity of the layout code. You'd have to run another pass over the layout system, as I mentioned.

We have a long list of things to do, and this feature which will only annoy people is a waste of time.

[WalterBright]

I did spend today looking at ways to refactor it into something simple, but with only minimal improvement.

[rikkimax]

The assumption we've been working with is that it's a simple matter of looking at a set of field type size + number of bits pair to detect the bad cases.

Something like this:

uint runningUsedBits;
uint maxInStorage;

foreach(field; *str.fields) {
    BitFieldDeclaration bfd = field.isBitFieldDeclaration;
    if (!bfd) {
        maxInStorage = 0;
        continue;
    }
   const width = bfd.fieldWidth;
   const size = bfd.type.size;

   if (maxInStorage == 0) {
       runningUsedBits = 0;
       maxInStorage = size;
   }

   runningUsedBits += width;

    if (runningUsedBits > maxInStorage)
        warn;
    maxInStorage = 0;
}

Am I right in thinking that you are thinking of something very different when you talk about running another pass?

[WalterBright]

All I can say is take a look at the implementation of setFieldOffset(). (The layout of a bitfield depends on the layout of the previous fields.)

[rikkimax]

I'm reading it, it's adjusting the properties like bit field offset and it's using intermediary data to do so.

What information do you need that is specific to a bitfield, that isn't its bit offset + width + alignment/storage size?

Nothing I am seeing suggests the need to rerun SetFieldOffsetVisitor over all the fields.

[WalterBright]

The field prior to it which affects it.

[rikkimax]

Does this have all the properties you need accessible to detect non-portability?

bool isStructBitFieldSafe(StructDeclaration sd) {
    uint runningUsedBits;
    uint maxInStorage;
    uint prevNonBitFieldOffset;
    uint prevNonBitFieldSize;
    
    foreach(field; *sd.fields) {
         VarDeclaration vd = field.isVarDeclaration;
          if (!vd)
             continue;
  
          const offset = vd.offset;
          const size = vd.type.size;
  
          BitFieldDeclaration bfd = field.isBitFieldDeclaration;
          if (!bfd) {
              maxInStorage = 0;
              prevNonBitFieldOffset = offset;
              prevNonBitFieldSize = size;
              continue;
          }
  
          const width = bfd.fieldWidth;
          
          if (maxInStorage == 0) {
              runningUsedBits = 0;
              maxInStorage = target.fieldalign(...);

              // check first bit field against prevNonBitFieldSize and prevNonBitFieldOffset
          } else {
              // check for bit field > 0, to see if it crosses storage boundary
              if (runningUsedBits + width > maxInStorage) {
                  error;
                  return false;
              }
          }
          
          runningUsedBits += width;

          if (runningUsedBits == maxInStorage)
              runningUsedBits = 0;
    }

    return true;
}

[WalterBright]

It depends on the alignment of the previous fields:

struct S { char c; int a:3; }

gives differing results. Look at the code to setFieldOffset(). You'd have to duplicate its every nuance. You'd also have to walk all the declarations in the module, i.e. it's another pass.

[WalterBright]

It's also important to not give an error message when the layout happens to be the same even though different rules are followed.

[rikkimax]

#20848

[ibuclaw]

For my own benefit, what is meant by non-portable layouts?

struct empty {}

Is a non-portable layout between extern(C) and extern(C++), for example.

[rikkimax]

For my own benefit, what is meant by non-portable layouts?

struct empty {}

Is a non-portable layout between extern(C) and extern(C++), for example.

For a given ABI, all the different layout engines for bitfields will agree on bit padding.

The simplest solution to that (which I implemented) is to simply ban any compiler-added padding.

[ibuclaw]

For my own benefit, what is meant by non-portable layouts?

struct empty {}

Is a non-portable layout between extern(C) and extern(C++), for example.

For a given ABI, all the different layout engines for bitfields will agree on bit padding.

The simplest solution to that (which I implemented) is to simply ban any compiler-added padding.

Different layout engines meaning dmd interfacing with gcc, clang, msvc? Compared with ldc/gdc who only interface with themselves?

[rikkimax]

Different layout engines meaning dmd interfacing with gcc, clang, msvc? Compared with ldc/gdc who only interface with themselves?

Interfacing? The frontend would have its own implementation of each engines strategy.

And each C compiler could support multiple (i.e. clang certainly supports MSVC and GCC's).