[Bug target/81273] Wrong code generated for ARM setting volatile struct field with a literal
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81273 Richard Earnshaw changed: What|Removed |Added Status|WAITING |RESOLVED Resolution|--- |INVALID --- Comment #6 from Richard Earnshaw --- The compiler gave you what you asked for (though it might not have been what you wanted, or even expected). Optimization is irrelevant here. Just because the address you finally assigned to the structure was aligned doesn't mean that the compiler can change the access pattern for a volatile object. You'd hardly expect that changing base_addr to 0x3f01 would change the sequence of loads and stores generated, surely? And don't forget that unaligned accesses to DEVICE memory are undefined.
[Bug target/81273] Wrong code generated for ARM setting volatile struct field with a literal
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81273
--- Comment #5 from LdB ---
Okay I understand all that and accept my apology and I will try to follow the
rules exactly in future.
I had to change the alignment command slightly to be valid on ARM 6.3.1
struct __attribute__((__packed__,aligned(__alignof(uint32_tTimerRegisters {
volatile uint32_t Load; //0x00
volatile uint32_t Value; //0x04
};
I am getting viable code with that, so it's undeniable it is an alignment
issue.
I am happy to have that as the solution and I understand it is the most
technically correct. It probably needs to be added to the manual because I
suspect it is going to come up a lot. I have never had the compiler spit
unaligned code as the most optimized code before.
[Bug target/81273] Wrong code generated for ARM setting volatile struct field with a literal
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81273 --- Comment #4 from Richard Earnshaw --- (In reply to LdB from comment #3) > I am stunned you could not build the code the only requirement is you > include the stdint.h so the uint32_t types are defined. I will fix the typos > are you really saying you can't compile this? > Most compiler developers do not install a complete build environment when testing bugs. As such, they do not have the headers available. This is why our bug reporting rules ask you to post _preprocessed_ source code. You've also failed to give: The options used to configure the compiler when you built it (see the output of gcc -v). The list of options you passed to the compiler when running it. We can only infer -O2 from your comments. > Alignment is not really a concern it's a hardware register to the CPU so > it's a given. Absolutely alignment is the issue here. Packed says that the object can be on any alignment boundary and that materially affects the compiler's behaviour here.
[Bug target/81273] Wrong code generated for ARM setting volatile struct field with a literal
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81273
--- Comment #3 from LdB ---
I am stunned you could not build the code the only requirement is you include
the stdint.h so the uint32_t types are defined. I will fix the typos are you
really saying you can't compile this?
#include
struct __attribute__((__packed__)) TimerRegisters {
volatile uint32_t Load; //0x00
volatile uint32_t Value; //0x04
};
uint32_t base_addr = 0x3F00;
#define TIMER ((struct TimerRegisters*)(base_addr + 0xB400))
void ShowBug (void){
TIMER->Load = 0x400; // -02 compile and look at code
}
void ShowFix1 (void){
*(volatile uint32_t*)TIMER->Load = 0x400;
}
void ShowFix2 (void){
volatile uint32_t temp = 0x400;
TIMER->Load = temp;
}
Alignment is not really a concern it's a hardware register to the CPU so it's a
given.
[Bug target/81273] Wrong code generated for ARM setting volatile struct field with a literal
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81273 Richard Biener changed: What|Removed |Added Status|UNCONFIRMED |WAITING Last reconfirmed||2017-07-03 Ever confirmed|0 |1 --- Comment #2 from Richard Biener --- Your testcase cannot be built. On GIMPLE ShowBug looks ok so the issue must be at RTL expansion. _NOTE_ that *(volatile uint32_t*) makes alignment guarantees to the access while your TimerRegisters struct is aligned at byte boundary only! I suppose you may want __attribute__((__packed__,aligned(alignof(uint32_t)?
[Bug target/81273] Wrong code generated for ARM setting volatile struct field with a literal
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81273 --- Comment #1 from LdB --- In above code the ARMTIMER cases should be TIMER not related to bug just cut and paste typo.
