On Mon, Jul 23, 2012 at 11:19 PM, Paul Voith <p...@voithconsulting.com> wrote:
> Peter,
>
> The change which caused code to compile differently involved the use of the
> (currently unsupported) iostructures.h file. Specifically the file contains
> the following definition to define a union of a bit field structure and an
> unsigned char:
>
> #ifdef MSP430_BITFIELDS_UNSIGNED
> #define __MSP430_UNS__
> #else
> #define __MSP430_UNS__ unsigned
> #endif
>
> #ifndef __MSP430_PORT_INIT_
> #define __VOLATILE volatile
> #else
> #define __VOLATILE
> #endif
>
> typedef union port {
> __VOLATILE __MSP430_UNS__ char reg_p;
> __VOLATILE struct {
> __MSP430_UNS__ char __p0:1,
> __p1:1,
> __p2:1,
> __p3:1,
> __p4:1,
> __p5:1,
> __p6:1,
> __p7:1;
> } __pin;
> } __attribute__ ((packed)) ioregister_t;
>
> #undef __MSP430_UNS__
>
> When I compile this in to my code the ioregister_t type has a size of 2
> bytes rather than 1 as if the incorporation of a bit field automatically
> promoted the type to int. Note that the "packed" attribute has been applied.
Yes, in October 2008 the structure alignment requirements of mspgcc
were changed to use word alignment by default. This was a good thing,
as the code produced when the compiler can't make that assumption is
really horrible (every word read from a structure through a pointer
has to be done byte-at-a-time).
In your example, you need to add the packed attribute to the interior
struct that has the bitfields for the pin. Then it should work fine.
That the union itself is packed doesn't affect the alignment
requirements imposed by its members. The same issue would arise if
the struct had one member that was an unsigned char; the bitfields are
a red herring.
> This type is used subsequently in the same header to construct register
> sets for example:
>
> #if defined(__msp430_have_port1) || defined(__msp430_have_port2)
> struct port_full_t {
> ioregister_t in; /* Input */
> ioregister_t out; /* Output */
> ioregister_t dir; /* Direction */
> ioregister_t ifg; /* Interrupt Flag */
> ioregister_t ies; /* Interrupt Edge Select */
> ioregister_t ie; /* Interrupt Enable */
> ioregister_t sel; /* Selection */
> };
> #endif
>
> A port is then defined with this structure using a constant offset:
>
> #ifdef __msp430_have_port1
> __MSP430_EXTERN__ struct port_full_t port1 asm("0x0020");
> #endif
>
> The problem arises in that the second member ("out") of the port structure
> now has an offset of 0x0022 since the ioregister_t type is 2 bytes rather
> than its expected offset of 0x0021. The result for me was that code which
> used these structures to reference "out" were now pointing to what would
> have been "dir" instead.
>
> This of course has nothing to do with my assertion that bit fields were at
> one time a mechanism to effect single instruction RMW cycles. You have
> certainly made me doubt that it was ever so. I now wonder if I am cross
> coupled in my mind with the TI paper on its TMS320 series processors
> "Programming TMS320x28xx and 28xxx Peripherals in C/C++ (SPRAA85)" which
> goes on about the advantages of bit field mapped registers and states:
>
> When writing to a bit field, the compiler generates what is called a
> read-modify-write assembly
> instruction. Read-modify-write refers to the technique used to implement
> bit-wise or byte-wise
> operations such as AND, OR and XOR. That is, the location is read, the
> single bit field is modified, and
> the result is written back.
>
> That is both a different processor and a different compiler but it is a case
> in which TI sanctioned the use of the bit field mapped register sets and it
> was my recollection (which I now doubt) that this was also applied to the
> MSP430 and then maybe only with code composer studio or the like.
I suspect the intent of that was to reassure the reader that the
compiler would be able to detect that situation and generate an RMW
instruction, and that bitfields should not be avoided because of an
assumption that they were poorly implemented. I don't think I would
read that as saying bit fields were what enabled the optimization, or
that the compiler could be relied upon to do anything special as a
consequence of bitfields being used. Of course my interpretation is
also biased by my own expectations; CCS might very well do something
special.
> So to be clear... I never meant that bit fields OUGHT to be used to
> implement single instruction RMW cycles. All that was understood was that IF
> you used the pre-defined bit fields then the compiler could be relied upon
> to generate a single instruction RMW. That may well have been due to the
> fact that such definitions were always volatile rather than to do with them
> being bit fields per se. Which leads me to something else you said:
>
> (There was a horribly grotesque hack intended
> to ensure RMW operations on volatiles got translated to single
> instructions, but it had nothing to do with bitfields and was
> demonstrably broken with complex expressions. I replaced it with a
> different solution that has not yet been proven to do anything
> incorrect.)
>
> Do you mean that you replaced it with a different solution which also
> translates RMW operations to single instructions or with a different
> solution which may or may not?
What the old solution did was attempt to disable GCC's enforcement of
the semantics of "volatile" in certain situations in hopes that the
code generated would not have extraneous reads. The consequence of
this was that situations where "volatile" was disregarded, resulting
in incorrect behavior. A simple example and some discussion is at
http://sourceforge.net/tracker/index.php?func=detail&aid=3399383&group_id=42303&atid=432701.
I ripped out that code after further investigation when I found a
more complex example that mis-ordered output pin settings.
In the current implementation, gcc is allowed to process volatile
according to its best understanding, and after all that's done a
peephole optimization pass is run which translates sequences of
load-modify-store instructions to the RMW version if doing so does not
affect the program function. This way I leave correctness of volatile
management to the gcc maintainers, and the benefits of the peephole
optimization apply to programs regardless of whether "volatile" is
involved.
> Also: I really do appreciate the your time responding to this. I have a lot
> of respect for all of you who maintain this compiler.
I thank you, and will forward your appreciation to me and myself.
Peter
>
>
> On 7/23/2012 3:24 PM, Peter Bigot wrote:
>
> On Mon, Jul 23, 2012 at 2:49 PM, Paul Voith <p...@voithconsulting.com>
> wrote:
>
> Peter,
>
> I agree that bitfields are a pain. The standards guarantee almost nothing
> regarding their internal implementation and so there is really no obvious
> reason to implement what amounts to a hack using them.
>
> While use of the a |= b may generate a single instruction IF b is a
> constant, it is not guaranteed and in cases where b is an arbitrary
> expression typically does not. The left side value is read; the expression
> on the right calculated, the already read left side expression is OR'd in
> and the the result written. Entering the expression for the right side into
> a variable prior to ORing is likely to be removed by the compiler
> optimization so nothing to rely on.
>
> Yes. I'm confused because, as you note in the previous paragraph,
> there's nothing guaranteed about the behavior of bitfields either. So
> I'm not seeing what's changed here. As I explain below, I see no
> evidence mspgcc ever did anything special here.
>
> Presently I notice that the 4.6.3 version of the compiler I am using accepts
> bitfield definitions without generating an error. For bitfields defined on a
> char or unsigned char base type it appears that bits so defined are packed
> into a 16 bit short word. This is a change from early versions but appears
> to have occurred well before 2008. This breaks the earlier include files
> which used to define these as bit fields within an unsigned char object. So
> legacy code compiles without error but generates an unexpected result. So
> even if TI provided such files the present bitfield implementation could not
> use them.
>
> I believe there are situations where you'd need to add the attribute
> "packed" to the structure declaration. "packed" is a standard gcc
> attribute and is documented in the gcc manual. In other situations
> that would not be necessary.
>
> Since the C/C++ standards (and most HLLs) generally presume that memory does
> not change value by itself and that reading from a memory location has no
> consequence, I/O register access is problematic. The volatile modifier
> allows handling of the first case of course by demanding that every explicit
> reference to an identifier in an expression forces an actual read of the
> associated memory location. [Aside: I wondered if it is allowed to read such
> a location MORE times than explicitly referenced?].
>
> With volatile? No, it's not allowed.
>
> Rather than trying to hack up the compiler it is probably best to just
> generate an inline assembly instruction sequence which performs the needed
> functionality.
>
> I use msp430-gcc a lot in my work. I have actually been using an extremely
> old version of the compiler and discovered the change when I recompiled code
> (without error) which simply didn't work as before.
>
> Can you provide a concrete example?
>
> I can say that I
> consider this ability to fiddle bits without semaphore protection of every
> register a huge convenience.
>
> Sure. But there's nothing about bitfields that makes that more
> likely, and some things about them that makes it less likely.
>
> In my maintenance and evolution of the mspgcc port I haven't seen any
> indication that mspgcc ever did anything to enable what you have come
> to expect. Certainly there were no traces left by the time mspgcc4
> came into the picture. (There was a horribly grotesque hack intended
> to ensure RMW operations on volatiles got translated to single
> instructions, but it had nothing to do with bitfields and was
> demonstrably broken with complex expressions. I replaced it with a
> different solution that has not yet been proven to do anything
> incorrect.)
>
> In short, I can't see why you think the bitfield structures ever
> provided the promises you've described.
>
> With the current version of the compiler, things work pretty reliably
> using standard C operations on standard data types, and I've not
> encountered any situation where a semaphore protecting a single
> register access was necessary. If you have found situations where
> things don't work, please file bug reports.
>
> Also (and of less concern actually): even though a case has been made that
> bitfields do not result in efficient code I think it is more that it lends
> itself to inefficient program design. When I write |= and &= versions of
> code which duplicate bitfield behavior they result in almost identical code
> IF you make the code do identical things. So X.bit3 = Y is really X |= (Y &
> 1) << 3 for example. IF Y is a constant both will result in a single
> instruction.
>
> Yes. gcc translates all bitfield operations into sequences of shifts
> and bitwise operations on values in the native word size in the middle
> end, long before the target gets a chance to do anything. At best
> they're a convenience to users, and at worst introduce extra steps the
> compiler has to work around (and sometimes fails to do so).
>
> Peter
>
> On 7/23/2012 5:15 AM, Peter Bigot wrote:
>
> While that may or may not have been the understanding of users of mspgcc
> during its earlier years, from my experience with GCC's internals any such
> an assumption was a mistake. Today, you are far more likely to have "atomic"
> RMW instructions if you use the standard C operators on natural integral
> types than if you use bitfields. Bitfields are complex to implement
> correctly, and gcc introduces overhead during analysis and optimization to
> ensure it doesn't violate the language semantics at a stage when it isn't
> concerned with target-specific capabilities. Google for "gcc bitfield"
> reveals many examples where this produces hideous code on a variety of
> targets.
>
> In code with any reasonable level of optimization (-O, -Os), mspgcc should
> generate single instruction implementations for & and | on standard C types
> wherever supported by the underlying ISA. When it does not, this is a bug
> that should be reported so it can be fixed. I believe other compilers
> (llvm?) might make other promises, or provide an alternative solution (such
> as intrinsics that are guaranteed to produce bic and bis instructions). I
> would entertain a proposal to add such intrinsics. Bitfield structures will
> be added only if the definitions are provided by TI in the headers shared
> among all MSP430 compilers.
>
> Peter
>
>
------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Mspgcc-users mailing list
Mspgcc-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mspgcc-users
