This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] reimplement -fstrict-volatile-bitfields v4, part 2/2


> where in the C standard did you read the requirement that every bit
> field must be complete? (This is a serious question).

The spec doesn't say each field must be complete, but neither does it
say that the structure must be as big as the type used.  If you
specify "int foo:1" then the compile is allowed to make the struct
smaller than "int".

> extern struct
> {
>   volatile unsigned int b : 1;
> } bf3;
> 
> on my compiler this structure occupies 4 bytes.
> and it is aligned at 4 bytes.
> That is OK for me and AAPCS.

On my target, the structure occupies 1 byte.  I suspect gcc is
rounding up to WORD_MODE, which is 4 bytes for you and 1 for me.  If
so, you're relying on a coincidence, not a standard.

> But the access "bf3.b=1" is SI mode with Sandra's patch (part 1, which just
> obeys the AAPCS and does nothing else) 
> and QI mode without this patch, which is just a BUG.
> I am quite surprised how your target manages to avoid it?

It doesn't, but I wouldn't expect it to, because IMHO that test is
invalid - you have not given a precise enough test to expect
consistent results.

> It is as Sandra said, at least on ARM -fstrict-volatile-bitfields
> does not function at all. And the C++11 memory model wins all the time.

Are we talking about N2429?  I read through the changes and it didn't
preclude honoring the user's types.

> > Looking through the tests, most of them combine "packed" with
> > mismatched types. IMHO, those tests are invalid.
> 
> I dont think so. They are simply packed. And volatile just says that
> the value may be changed by a different thread. It has a great
> impact on loop optimizations.

Nothing you've said so far precludes honoring the user's types when
the user gives you a consistent structure.  Consider:

typedef struct { unsigned char b0:1, b1:1, b2:1, b3:1, b4:1, b5:1, b6:1, b7:1; } Bits;

extern volatile struct {
  Bits reg1; /* read status without resetting them */
  Bits reg2; /* read status and atomically reset them */
} interrupt_handler;

Without -fstrict-volatile-bitfields, gcc is allowed to use a 16-bit
access to read reg1, but this causes an unexpected read to volatile
reg2, which may have unintended consequences.  The spec doesn't say
the compiler *must* read reg2, it just *doesn't* say that it *can't*.
The -fstrict-volatile-bitfields flag tells the compiler that it
*can't*.  IMHO this doesn't violate the spec, it just limits what the
compiler can do within the spec.

If ARM wants fast word-sized volatile bitfields, use "int" and
structure your bitfields so that no "int" field overlaps a natural
"int" boundary.

When I added the option, I considered making it an error() to define a
strict volatile bitfield that spanned the allowed boundary of the type
specified, but I figured that would be too much.

> > I've not objected to fixing -fstrict-volatile-bitfields, or making the
> > -fno-strict-volatile-bitfields case match the standard. I've only
> > objected to breaking my targets by making that flag not the default.
> 
> Fine. Why cant we just get this fixed?

Dunno.  I'm only opposing the patch that disabled it on my targets.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]