public inbox for gcc@gcc.gnu.org
 help / color / mirror / Atom feed
* a feature to the wishlist
@ 2021-09-13  6:55 Rafał Pietrak
  2021-09-13  8:44 ` Jonathan Wakely
  0 siblings, 1 reply; 6+ messages in thread
From: Rafał Pietrak @ 2021-09-13  6:55 UTC (permalink / raw)
  To: gcc

Hi everybody,

I've been using GCC for a varety of ARM micro controller project. With
embedded systems, one often has to access specific memory locations,
which contain hardware registers, that control device behavior.
"traditionally" or "commonly" that's codded by programmers using
"#define", thus relaying on preprocessor to put specific compiler
constructs, that do the job.

IMHO, this is wrong exactly because of the involvement of preprocessor.
Taking as an example exerpts from libopencm3, like:
	>> USB_SET_EP_RX_STAT(addr, USB_EP_RX_STAT_VALID)
one can understand, that such "code" can actually translate to quite
anything.

Getting to the point, here is code exerpt of by own STM32 usart driver:
------------------------------------------------------------------
#define USART_RE	11
#define USART_RENEIE	8
#define USART_TE	10
#define USART_TXEIE	6
#define USART_UE	0

extern struct usart_s {
	uint sr;
	uint dr;			// offset 0x04
	uint brr;			// offset 0x08
	volatile union { uint cr1;	// offset 0x0C
		struct cr_s { uint sbk:1, rwu:1,
		re:1, te:1, idleie:1, rxneie:1,
		tcie:1, txeie:1, peie:1, ps:1,
		pce:1, wake:1, m:1, ue:1;} cr1_s;
	};
} USART1;

static void EXIT(void *tty) {
	volatile struct usart_s *d = &USART1;

#if VARIANT_TRADITIONAL
	d->cr1 &= ~(1 << USART_RE) & ~(1 << USART_RXNEIE)
		& ~(1 << USART_TE) & ~(1 << USART_TXEIE)
		& ~(1 << USART_UE);
#elif VARIANT_WORKING
	struct cr_s a = (struct cr_s) {
			.re = 1,
			.te = 1,
       			.rxneie = 1,
			.txeie = 1,
			.ue = 1 };
	int *b = (int *) &a;
	d->cr1 &= ~(*b);
#elif VARIANT_BETTER
	(union cr_u) d->cr1 &= ~ {
			.re = 1,
			.te = 1,
       			.rxneie = 1,
			.txeie = 1,
			.ue = 1 };
#elif VARIANT_THE_BEST
	d->cr1_s &= ~ { .re = 1,
			.te = 1,
       			.rxneie = 1,
			.txeie = 1,
			.ue = 1 };
#endif
}
----------------------------------------------------------------

In function EXIT, you can see four variants of "the same code". First
fragment is codded just like todays common practice is. This is BAD
coding, because debugger has no way of presenting "the real thing".

Using gcc-8-branch revision 273027 (arm-none-eabi-gcc linux cross
compiler) I was able to code the very same thing by VARIANT_WORKING,
which is far better to read (IMHO). The disadvantage is, that I had to
use variable "b" to fool gcc syntax parser. No direct multiple "type
overwrite" worked for me.

Then again, it would be even better (and easier to read the code),
should gcc syntax parser recognised VARIANT_BETTER or VARIANT_THE_BEST
as "syntactic sugar" to generate code, that currently gets issued by
VARIANT_WORKING (being the same as issued by VARIANT_TRADITIONAL, which
is very OK).

Pls CC: responses to my address, as I'm not a subscriber to the gcc list.

So my question to the list is: are there chances, the issue can be
recognised as important, and planed for some future gcc?

with best regards,

Rafał Pietrak

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: a feature to the wishlist
  2021-09-13  6:55 a feature to the wishlist Rafał Pietrak
@ 2021-09-13  8:44 ` Jonathan Wakely
  2021-09-13  9:51   ` Rafał Pietrak
  0 siblings, 1 reply; 6+ messages in thread
From: Jonathan Wakely @ 2021-09-13  8:44 UTC (permalink / raw)
  To: Rafał Pietrak; +Cc: gcc

On Mon, 13 Sept 2021 at 07:55, Rafał Pietrak via Gcc <gcc@gcc.gnu.org> wrote:
>
> Hi everybody,
>
> I've been using GCC for a varety of ARM micro controller project. With
> embedded systems, one often has to access specific memory locations,
> which contain hardware registers, that control device behavior.
> "traditionally" or "commonly" that's codded by programmers using
> "#define", thus relaying on preprocessor to put specific compiler
> constructs, that do the job.
>
> IMHO, this is wrong exactly because of the involvement of preprocessor.
> Taking as an example exerpts from libopencm3, like:
>         >> USB_SET_EP_RX_STAT(addr, USB_EP_RX_STAT_VALID)
> one can understand, that such "code" can actually translate to quite
> anything.
>
> Getting to the point, here is code exerpt of by own STM32 usart driver:
> ------------------------------------------------------------------
> #define USART_RE        11
> #define USART_RENEIE    8
> #define USART_TE        10
> #define USART_TXEIE     6
> #define USART_UE        0
>
> extern struct usart_s {
>         uint sr;
>         uint dr;                        // offset 0x04
>         uint brr;                       // offset 0x08
>         volatile union { uint cr1;      // offset 0x0C
>                 struct cr_s { uint sbk:1, rwu:1,
>                 re:1, te:1, idleie:1, rxneie:1,
>                 tcie:1, txeie:1, peie:1, ps:1,
>                 pce:1, wake:1, m:1, ue:1;} cr1_s;
>         };
> } USART1;
>
> static void EXIT(void *tty) {
>         volatile struct usart_s *d = &USART1;
>
> #if VARIANT_TRADITIONAL
>         d->cr1 &= ~(1 << USART_RE) & ~(1 << USART_RXNEIE)
>                 & ~(1 << USART_TE) & ~(1 << USART_TXEIE)
>                 & ~(1 << USART_UE);
> #elif VARIANT_WORKING
>         struct cr_s a = (struct cr_s) {
>                         .re = 1,
>                         .te = 1,
>                         .rxneie = 1,
>                         .txeie = 1,
>                         .ue = 1 };
>         int *b = (int *) &a;
>         d->cr1 &= ~(*b);

This is a strict aliasing violation. You should either use a union or
memcpy to get the value as an int.

> #elif VARIANT_BETTER
>         (union cr_u) d->cr1 &= ~ {
>                         .re = 1,
>                         .te = 1,
>                         .rxneie = 1,
>                         .txeie = 1,
>                         .ue = 1 };
> #elif VARIANT_THE_BEST
>         d->cr1_s &= ~ { .re = 1,
>                         .te = 1,
>                         .rxneie = 1,
>                         .txeie = 1,
>                         .ue = 1 };
> #endif
> }
> ----------------------------------------------------------------
>
> In function EXIT, you can see four variants of "the same code". First
> fragment is codded just like todays common practice is. This is BAD
> coding, because debugger has no way of presenting "the real thing".

Have you tried -ggdb3 which does preserve macro information?

> Using gcc-8-branch revision 273027 (arm-none-eabi-gcc linux cross
> compiler) I was able to code the very same thing by VARIANT_WORKING,
> which is far better to read (IMHO). The disadvantage is, that I had to
> use variable "b" to fool gcc syntax parser. No direct multiple "type
> overwrite" worked for me.
>
> Then again, it would be even better (and easier to read the code),
> should gcc syntax parser recognised VARIANT_BETTER or VARIANT_THE_BEST
> as "syntactic sugar" to generate code, that currently gets issued by
> VARIANT_WORKING (being the same as issued by VARIANT_TRADITIONAL, which
> is very OK).

You can define an inline function that initializes the struct, then
returns the negation of the int value.

I very much doubt GCC will add an extension to make ~ work on structs.
What would it even mean if sizeof(struct cr_s) == 3 or == 1024?

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: a feature to the wishlist
  2021-09-13  8:44 ` Jonathan Wakely
@ 2021-09-13  9:51   ` Rafał Pietrak
  2021-09-13 11:41     ` David Brown
  0 siblings, 1 reply; 6+ messages in thread
From: Rafał Pietrak @ 2021-09-13  9:51 UTC (permalink / raw)
  To: Jonathan Wakely; +Cc: gcc

Hi,

Thenk you for very prompt reply.

W dniu 13.09.2021 o 10:44, Jonathan Wakely pisze:
> On Mon, 13 Sept 2021 at 07:55, Rafał Pietrak via Gcc <gcc@gcc.gnu.org> wrote:
[-----------------]
>> #elif VARIANT_WORKING
>>         struct cr_s a = (struct cr_s) {
>>                         .re = 1,
>>                         .te = 1,
>>                         .rxneie = 1,
>>                         .txeie = 1,
>>                         .ue = 1 };
>>         int *b = (int *) &a;
>>         d->cr1 &= ~(*b);
> 
> This is a strict aliasing violation. You should either use a union or
> memcpy to get the value as an int.

Yes, I know. I know, this is a "trick" I use (I had to use to missleed
gcc).

I've put it here as an example of something, that allows me to get the
very same binary output as the "traditional" coding, .... while (IMHO)
improving readability of the code.

[--------------]
>> In function EXIT, you can see four variants of "the same code". First
>> fragment is codded just like todays common practice is. This is BAD
>> coding, because debugger has no way of presenting "the real thing".
> 
> Have you tried -ggdb3 which does preserve macro information?

No. I'll check it out. Thenx. Still, gdb translation of macros is just
one of the problems with traditioonal coding (explanation followes).

[------------]
>> Then again, it would be even better (and easier to read the code),
>> should gcc syntax parser recognised VARIANT_BETTER or VARIANT_THE_BEST
>> as "syntactic sugar" to generate code, that currently gets issued by
>> VARIANT_WORKING (being the same as issued by VARIANT_TRADITIONAL, which
>> is very OK).
> 
> You can define an inline function that initializes the struct, then
> returns the negation of the int value.

Hmmm. probably yes. But this will only obfuscate the code. I'm trying to
say here, that part of the consequence of missing support for constructs
like VARIANT_THE_BEST, is that todays coding practice is exactly like
this. Meaning: people are doing those inline functions, and to be able
to distinguish them, call them like:
[now_lets_disable_receiving_register()] .... which in reality is putting
a comment into function name. IMHO - a bad thing. A comment is a comment.

I hopped, that support for direct (explicit) struct operations like in
my example, could ultimately streighten the practice. But I can see from
your reply that there is very little chance of that.

> 
> I very much doubt GCC will add an extension to make ~ work on structs.
> What would it even mean if sizeof(struct cr_s) == 3 or == 1024?

And that's a very good point. ... and in favor of my arguments.

The [struct cr_s] is defined in CPU documentation: it is, and it must be
4. Should a programmer make a typo declaring the struct, a typo which
would result in such sizeof mismatch, the compiler could raise an alarm.
With the proposed construct, the compiler can do that. That's very
desirable. And I'm speaking from experience. I did have this sort or
errors myself, and VARIANT_TRADITIONAL allowed me that without a glitch.

But if nothing can be done, that's life. Thank you anyway.

with best regards,

-R

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: a feature to the wishlist
  2021-09-13  9:51   ` Rafał Pietrak
@ 2021-09-13 11:41     ` David Brown
  2021-09-14 18:48       ` Rafał Pietrak
  0 siblings, 1 reply; 6+ messages in thread
From: David Brown @ 2021-09-13 11:41 UTC (permalink / raw)
  To: Rafał Pietrak, Jonathan Wakely; +Cc: gcc

On 13/09/2021 11:51, Rafał Pietrak via Gcc wrote:
> Hi,
> 
> Thenk you for very prompt reply.


(I'm not sure how much this is a "gcc development list" matter, rather
than perhaps a "gcc help list" or perhaps comp.arch.embedded Usenet
group discussion, but I'll continue here until Jonathan or other gcc
developers say stop.)

> 
> W dniu 13.09.2021 o 10:44, Jonathan Wakely pisze:
>> On Mon, 13 Sept 2021 at 07:55, Rafał Pietrak via Gcc <gcc@gcc.gnu.org> wrote:
> [-----------------]
>>> #elif VARIANT_WORKING
>>>         struct cr_s a = (struct cr_s) {
>>>                         .re = 1,
>>>                         .te = 1,
>>>                         .rxneie = 1,
>>>                         .txeie = 1,
>>>                         .ue = 1 };
>>>         int *b = (int *) &a;
>>>         d->cr1 &= ~(*b);
>>
>> This is a strict aliasing violation. You should either use a union or
>> memcpy to get the value as an int.
> 
> Yes, I know. I know, this is a "trick" I use (I had to use to missleed
> gcc).
> 

Don't think of it as a "trick" - think of it as a mistake.  A completely
unnecessary mistake, that will likely give you unexpected results at times :

    union {
	struct cr_s s;
        uint32_t raw;
    } a = {(struct cr_s) {
	.re = 1,
	.te = 1,
      	.rxneie = 1,
	.txeie = 1,
	.ue = 1 }
    };
    d->cr1 &= ~(a.raw);


I hope you also realise that the "VARIANT_TRADITIONAL" and
"VARIANT_WORKING" versions of your code do different things.  The ARM
Cortex-M devices (like the STM-32) are little-endian, and the bitfields
are ordered from the LSB.  So either your list of #define's is wrong, or
your struct cr_s bitfiled is wrong.  (I haven't used that particular
device myself, so I don't know which is wrong.)

Also - perhaps beside the point, but good advice anyway - for this kind
of work you should always use the fixed-size <stdint.h> types such as
"uint32_t", not home-made types like "uint".  And you always want
unsigned types when doing bit operations such as bitwise complement.

Using a union of the bitfield struct with a "raw" combined field is a
common idiom, and gives you exactly what you need here.  It is
guaranteed to work in C, unlike your code (which has undefined
behaviour).  If it is important to you, you should note that it is not
defined behaviour in C++ (though maybe gcc guarantees it will work - the
documentation of "-fstrict-aliasing" is not clear on the matter).

As Jonathan says, a small inline function or macro (using gcc's
extension of declarations and statements inside an expression) can be
used for a wrapper that will work simply and efficiently while being
safe for both C and C++ :

#define raw32(x) \
	({ uint32_t raw; memcpy(&raw, &x, sizeof(uint32_t)); raw;})


    struct cr_s a = (struct cr_s) {
	.re = 1,
	.te = 1,
      	.rxneie = 1,
	.txeie = 1,
	.ue = 1 }
    };
    d->cr1 &= ~raw32(a);


mvh.,

David

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: a feature to the wishlist
  2021-09-13 11:41     ` David Brown
@ 2021-09-14 18:48       ` Rafał Pietrak
  2021-09-15  7:20         ` David Brown
  0 siblings, 1 reply; 6+ messages in thread
From: Rafał Pietrak @ 2021-09-14 18:48 UTC (permalink / raw)
  To: David Brown, Jonathan Wakely; +Cc: gcc



W dniu 13.09.2021 o 13:41, David Brown pisze:
> On 13/09/2021 11:51, Rafał Pietrak via Gcc wrote:
>> Hi,
>>
>> Thenk you for very prompt reply.
> 
> 
> (I'm not sure how much this is a "gcc development list" matter, rather
> than perhaps a "gcc help list" or perhaps comp.arch.embedded Usenet
> group discussion, but I'll continue here until Jonathan or other gcc
> developers say stop.)

Thank you. I appreciate it.

And yet, I do think, that it's about the "core" gcc - it's about a
programmer is able to "talk to gcc" without much "missunderstanding".

But I'm not going to push it much more. I accept, that "talking to gcc"
with d->cr1 &= ~ { ... } syntax is not what majority of programmers
would like to be able to do.

> 
>>
>> W dniu 13.09.2021 o 10:44, Jonathan Wakely pisze:
>>> On Mon, 13 Sept 2021 at 07:55, Rafał Pietrak via Gcc <gcc@gcc.gnu.org> wrote:
>> [-----------------]
>>>> #elif VARIANT_WORKING
>>>>         struct cr_s a = (struct cr_s) {
>>>>                         .re = 1,
>>>>                         .te = 1,
>>>>                         .rxneie = 1,
>>>>                         .txeie = 1,
>>>>                         .ue = 1 };
>>>>         int *b = (int *) &a;
>>>>         d->cr1 &= ~(*b);
>>>
>>> This is a strict aliasing violation. You should either use a union or
>>> memcpy to get the value as an int.
>>
>> Yes, I know. I know, this is a "trick" I use (I had to use to missleed
>> gcc).
>>
> 
> Don't think of it as a "trick" - think of it as a mistake.  A completely
> unnecessary mistake, that will likely give you unexpected results at times :
> 
>     union {
> 	struct cr_s s;
>         uint32_t raw;
>     } a = {(struct cr_s) {
> 	.re = 1,
> 	.te = 1,
>       	.rxneie = 1,
> 	.txeie = 1,
> 	.ue = 1 }
>     };
>     d->cr1 &= ~(a.raw);

Ha! This is very nice.

But pls note, that if contrary to my VARIANT_WORKING this actually is
kosher (and not an error, like you've said about the my "WORKING"), and
it actually looks very similar to the VARIANT_THE_BEST ... may be there
is a way to implement VARIANT_THE_BEST as "syntactic trick" leading
compiler into this semantics you've outlined above?

I'm raising this questions, since CR1 as int (or better as uint32_t) is
already declared in my code. Compiler shouldn't have too hard time
weeding out struct cr_s from union embedding it.

> 
> I hope you also realise that the "VARIANT_TRADITIONAL" and
> "VARIANT_WORKING" versions of your code do different things.  The ARM
> Cortex-M devices (like the STM-32) are little-endian, and the bitfields
> are ordered from the LSB.  So either your list of #define's is wrong, or
> your struct cr_s bitfiled is wrong.  (I haven't used that particular
> device myself, so I don't know which is wrong.)

I'm sory. this is my mistake. I've taken a shortcut and quickly written
an at hoc '#defines' for the email. In my code I have:
enum usart_cr1_e {
	USART_SBK, USART_RWU, USART_RE, USART_TE, USART_IDLEIE,
	USART_RXNEIE, USART_TCIE, USART_TXEIE, USART_PEIE, USART_PS,
	USART_PCE, USART_WAKE, USART_M, USART_UE,
};

And gcc produces exactly the same code in both variants:
 8000c56:	68d3      	ldr	r3, [r2, #12]
 8000c58:	f423 5302 	bic.w	r3, r3, #8320	; 0x2080
 8000c5c:	f023 032c 	bic.w	r3, r3, #44	; 0x2c
 8000c60:	60d3      	str	r3, [r2, #12]


> 
> Also - perhaps beside the point, but good advice anyway - for this kind
> of work you should always use the fixed-size <stdint.h> types such as
> "uint32_t", not home-made types like "uint".  And you always want
> unsigned types when doing bit operations such as bitwise complement.

Yes. That's true. No problem here.

> 
> Using a union of the bitfield struct with a "raw" combined field is a
> common idiom, and gives you exactly what you need here.  It is
> guaranteed to work in C, unlike your code (which has undefined
> behaviour).  If it is important to you, you should note that it is not
> defined behaviour in C++ (though maybe gcc guarantees it will work - the
> documentation of "-fstrict-aliasing" is not clear on the matter).

Somehow I've missed it until now, always falling back to things like
(int *) or even (void *). That's a good lesson for me.

> 
> As Jonathan says, a small inline function or macro (using gcc's
> extension of declarations and statements inside an expression) can be
> used for a wrapper that will work simply and efficiently while being
> safe for both C and C++ :
> 
> #define raw32(x) \
> 	({ uint32_t raw; memcpy(&raw, &x, sizeof(uint32_t)); raw;})
> 
> 
>     struct cr_s a = (struct cr_s) {
> 	.re = 1,
> 	.te = 1,
>       	.rxneie = 1,
> 	.txeie = 1,
> 	.ue = 1 }
>     };
>     d->cr1 &= ~raw32(a);

May be I'll test it in some future. For the time being I'm very
satisfied with the assignment to a temporary union variable.

but having said that, I strongly believe (particularly if union variable
is kosher), that gcc recognising VARIANT_THE_BEST as a syntax shortcut
to such union temporary variable could significantly improve code
readibility.

So thank you for extended explanation, and I'm currently staying off the
list hoping that my perspective will eventually catch some traction :)


Thank you again,

Rafał

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: a feature to the wishlist
  2021-09-14 18:48       ` Rafał Pietrak
@ 2021-09-15  7:20         ` David Brown
  0 siblings, 0 replies; 6+ messages in thread
From: David Brown @ 2021-09-15  7:20 UTC (permalink / raw)
  To: Rafał Pietrak, Jonathan Wakely; +Cc: gcc


On 14/09/2021 20:48, Rafał Pietrak wrote:
> 
> 
> W dniu 13.09.2021 o 13:41, David Brown pisze:
>> On 13/09/2021 11:51, Rafał Pietrak via Gcc wrote:
>>> Hi,
>>>
>>> Thenk you for very prompt reply.
>>
>>
>> (I'm not sure how much this is a "gcc development list" matter, rather
>> than perhaps a "gcc help list" or perhaps comp.arch.embedded Usenet
>> group discussion, but I'll continue here until Jonathan or other gcc
>> developers say stop.)
> 
> Thank you. I appreciate it.
> 
> And yet, I do think, that it's about the "core" gcc - it's about a
> programmer is able to "talk to gcc" without much "missunderstanding".
> 
> But I'm not going to push it much more. I accept, that "talking to gcc"
> with d->cr1 &= ~ { ... } syntax is not what majority of programmers
> would like to be able to do.

The gcc developers are always interested in new ideas.  But they are not 
keen on new syntaxes or extensions - there has to be a /very/ good 
reason for them.  The days of gcc being a maverick that makes up its own 
improved C are long, long gone - they'd much rather work with the C and 
C++ committees towards improving the languages in general, instead of 
having more of their own non-standard additions.  So an extension like 
this should ideally be a proposal for adding to C23, though gcc could 
always implement it earlier.  And while I appreciate what you are trying 
to do here, it is simply not general enough or important enough to 
justify such changes.  To get a new feature implemented, it has to do 
something you could not do before, or do it /far/ more simply, clearly, 
safely or efficiently.

> 
>>
>>>
>>> W dniu 13.09.2021 o 10:44, Jonathan Wakely pisze:
>>>> On Mon, 13 Sept 2021 at 07:55, Rafał Pietrak via Gcc <gcc@gcc.gnu.org> wrote:
>>> [-----------------]
>>>>> #elif VARIANT_WORKING
>>>>>          struct cr_s a = (struct cr_s) {
>>>>>                          .re = 1,
>>>>>                          .te = 1,
>>>>>                          .rxneie = 1,
>>>>>                          .txeie = 1,
>>>>>                          .ue = 1 };
>>>>>          int *b = (int *) &a;
>>>>>          d->cr1 &= ~(*b);
>>>>
>>>> This is a strict aliasing violation. You should either use a union or
>>>> memcpy to get the value as an int.
>>>
>>> Yes, I know. I know, this is a "trick" I use (I had to use to missleed
>>> gcc).
>>>
>>
>> Don't think of it as a "trick" - think of it as a mistake.  A completely
>> unnecessary mistake, that will likely give you unexpected results at times :
>>
>>      union {
>> 	struct cr_s s;
>>          uint32_t raw;
>>      } a = {(struct cr_s) {
>> 	.re = 1,
>> 	.te = 1,
>>        	.rxneie = 1,
>> 	.txeie = 1,
>> 	.ue = 1 }
>>      };
>>      d->cr1 &= ~(a.raw);
> 
> Ha! This is very nice.
> 
> But pls note, that if contrary to my VARIANT_WORKING this actually is
> kosher (and not an error, like you've said about the my "WORKING"), and
> it actually looks very similar to the VARIANT_THE_BEST ... may be there
> is a way to implement VARIANT_THE_BEST as "syntactic trick" leading
> compiler into this semantics you've outlined above?
> 

The issue I noticed with your "WORKING" is the bit order - that is 
orthogonal to the code structure, and easy to fix by re-arranging the 
fields in the initial bit-field.  It is independent from the structure 
of the code.

> I'm raising this questions, since CR1 as int (or better as uint32_t) is
> already declared in my code. Compiler shouldn't have too hard time
> weeding out struct cr_s from union embedding it.

The code to convert between a bit-field of size 32-bit and a uint32_t is 
nothing at all, and the compiler can handle that fine :-)  But you have 
to write the source code in a way that the conversion is well defined 
behaviour.  When you write something that is not defined behaviour, the 
compiler can generate code in ways you don't expect because technically 
you haven't told it what you actually want.

> 
>>
>> I hope you also realise that the "VARIANT_TRADITIONAL" and
>> "VARIANT_WORKING" versions of your code do different things.  The ARM
>> Cortex-M devices (like the STM-32) are little-endian, and the bitfields
>> are ordered from the LSB.  So either your list of #define's is wrong, or
>> your struct cr_s bitfiled is wrong.  (I haven't used that particular
>> device myself, so I don't know which is wrong.)
> 
> I'm sory. this is my mistake. I've taken a shortcut and quickly written
> an at hoc '#defines' for the email. In my code I have:
> enum usart_cr1_e {
> 	USART_SBK, USART_RWU, USART_RE, USART_TE, USART_IDLEIE,
> 	USART_RXNEIE, USART_TCIE, USART_TXEIE, USART_PEIE, USART_PS,
> 	USART_PCE, USART_WAKE, USART_M, USART_UE,
> };
> 
> And gcc produces exactly the same code in both variants:
>   8000c56:	68d3      	ldr	r3, [r2, #12]
>   8000c58:	f423 5302 	bic.w	r3, r3, #8320	; 0x2080
>   8000c5c:	f023 032c 	bic.w	r3, r3, #44	; 0x2c
>   8000c60:	60d3      	str	r3, [r2, #12]
> 
> 

Great.

>>
>> Also - perhaps beside the point, but good advice anyway - for this kind
>> of work you should always use the fixed-size <stdint.h> types such as
>> "uint32_t", not home-made types like "uint".  And you always want
>> unsigned types when doing bit operations such as bitwise complement.
> 
> Yes. That's true. No problem here.
> 
>>
>> Using a union of the bitfield struct with a "raw" combined field is a
>> common idiom, and gives you exactly what you need here.  It is
>> guaranteed to work in C, unlike your code (which has undefined
>> behaviour).  If it is important to you, you should note that it is not
>> defined behaviour in C++ (though maybe gcc guarantees it will work - the
>> documentation of "-fstrict-aliasing" is not clear on the matter).
> 
> Somehow I've missed it until now, always falling back to things like
> (int *) or even (void *). That's a good lesson for me.
> 

I always view any use of "void *" with suspicion - it is telling the 
compiler you don't know your real type.  It has its uses, of course, but 
I prefer to be more explicit when I can.  I would never use a cast to 
"int *" for accessing anything other than an int, and even with 
"uint32_t *" I'd normally have a "volatile" in there, and probably have 
an access function or wrapper too.  (With "volatile", the compiler will 
follow your instructions strictly - it lets you do messy casts safely, 
but can lead to inefficiencies.)

>>
>> As Jonathan says, a small inline function or macro (using gcc's
>> extension of declarations and statements inside an expression) can be
>> used for a wrapper that will work simply and efficiently while being
>> safe for both C and C++ :
>>
>> #define raw32(x) \
>> 	({ uint32_t raw; memcpy(&raw, &x, sizeof(uint32_t)); raw;})
>>
>>
>>      struct cr_s a = (struct cr_s) {
>> 	.re = 1,
>> 	.te = 1,
>>        	.rxneie = 1,
>> 	.txeie = 1,
>> 	.ue = 1 }
>>      };
>>      d->cr1 &= ~raw32(a);
> 
> May be I'll test it in some future. For the time being I'm very
> satisfied with the assignment to a temporary union variable.
> 
> but having said that, I strongly believe (particularly if union variable
> is kosher), that gcc recognising VARIANT_THE_BEST as a syntax shortcut
> to such union temporary variable could significantly improve code
> readibility.

Type-punning via unions is guaranteed to work in C, but undefined 
behaviour in C++.  (gcc may choose to define it, however.)

For the safest and most portable way of accessing raw memory, memcpy() 
always works.  gcc (and many other compilers) fully optimise memcpy() 
calls that have fixed sizes like this - the code generated here is as 
optimal as any other solution.  Some compilers, however, will give you a 
library call which is obviously not efficient.

gcc has another extension that can also be useful for this kind of thing:

	typedef uint32_t __attribute__((may_alias)) uint32_a;

Now you can safely use a cast to "uint32_a *", rather than "uint32_t *", 
and access data - gcc knows it may alias the struct :

	struct cr_s a = ....
	d->cr &= ~ *((uint32_a*) &a);


> 
> So thank you for extended explanation, and I'm currently staying off the
> list hoping that my perspective will eventually catch some traction :)
> 

This is still copied to the list.  If you want to look a bit at the 
list, without following everything, I recommend using the gmane.org 
Usenet gateway that provides a lot of mailing lists in convenient Usenet 
format.  There is plenty of interesting discussions on the gcc mailing 
lists.  I personally haven't done any gcc development - I'd love to do 
so, but haven't had the time to dig into it - but I sometimes get to 
help others and save the real developers some time.

mvh.,

David


> 
> Thank you again,
> 
> Rafał
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-09-15  7:20 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-13  6:55 a feature to the wishlist Rafał Pietrak
2021-09-13  8:44 ` Jonathan Wakely
2021-09-13  9:51   ` Rafał Pietrak
2021-09-13 11:41     ` David Brown
2021-09-14 18:48       ` Rafał Pietrak
2021-09-15  7:20         ` David Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).