[coreboot] GCC update broke AMD Fam10h boot

[Aaron Durbin]

On Thu, Mar 19, 2015 at 3:54 PM, Julius Werner <jwerner at chromium.org> wrote:
>> You are right that it would work, but back solving for which alignment
>> the compiler decided is hard. It's definitely whack-a-mole in that
>> case. It could have very well decided 16 too. Without any insight as
>> to why that breaks down. Or you go the route of putting alignments on
>> structures just for that behavior which I consider not the best
>> approach.
>
> Well, we only need to pick an alignment *larger* than what the
> compiler decides, right? I'd hazard a guess that 8 would be enough for
> a long time (GCC probably just got confused between 32 and 64 bit
> modes or something... there's no other reason I could think of to
> explain this).

Yes, my concern was not knowing that upper limit. I looked into the
Linux kernel on how they do most of this. From what I could tell they
largely use pointers to the structures like I worked around it. When
embedding structures into sections they use ALIGN(8) as you suggested
(see include/asm-generic/vmlinux.lds.h for the fun).

That said, I went back and looked at the assembly dump. It was using
0x14 as the size of the structure when sequencing through the entries.

001465dc R _bs_init_begin
001465e0 r cbmem_bscb
00146600 r gcov_bscb
0014663c R _bs_init_end

Each *entry* was aligned to 0x20.  Just aligning the symbol wouldn't
have fixed it. That means we'd need to mark the structure declaration
as aligned to a specific value and annotate it accordingly in the
linker script.  That's one route or just go w/ pointers to the
structures.

I'm looking forward to your linker scripts through preprocessor
patches to land because we can leverage for aligning some linker
scripts across archs, etc. Right now the changes are cumbersome. The
last part is just me whining. :)

>
>>> From that I'd conclude that if you did:
>>>
>>> extern struct boot_state_init_entry _bs_init_begin[];
>>> extern struct boot_state_init_entry _bs_init_end[];
>>>
>>> for (cur = _bs_init_begin; cur < _bs_init_end; cur++) ...
>>>
>>> the compiler shouldn't be able to guarantee that the first array
>>> wasn't empty and therefore pointing to "one after the end" which is
>>> the start of the next array in the first iteration.
>>
>> I think you are right on that point -- using '<' instead of equality.
>> But those aren't pointer types. Not sure if the spec things arrays
>> like that are poitners or a separate type. Either way, hopefully there
>> isn't some language in the spec that suggests comparisons like that
>> can be optimized away.
>
> The array degenerates to a pointer at that point AFAIK. You could also
> write &_bs_init_begin... for an array type the two should be pretty
> much equivalent.

We can still try that w/o the NULL tombstones. It doesn't hurt to try, right?