On Mon, 2023-08-07 at 11:41 +0200, Jo Van Bulck wrote: > On 28.07.23 21:19, Jarkko Sakkinen wrote: > > So, when exactly is it optimized away by the compiler? This is missing. > > The problem is that declaring encl_buf as static, implies that it will > only be used in this file and the compiler is allowed to optimize away > any entries that are never used within this compilation unit (e.g., when > optimizing out the memcpy calls). > > In reality, the tests outside test_encl.elf rely on both the size and > exact placement of encl_buf at the start of .data. > > For example, clang -Os generates the following (legal) code when > encl_bug is declared as static: > > 0000000000001020 <do_encl_op_put_to_buf>: > mov 0x8(%rdi),%al > mov %al,0x1fd7(%rip) # 3000 <encl_buffer.0> > mov 0x9(%rdi),%al > mov %al,0x8fce(%rip) # a000 <encl_buffer.1.0> > mov 0xa(%rdi),%al > mov %al,0x8fd5(%rip) # a010 <encl_buffer.1.1> > mov 0xb(%rdi),%al > mov %al,0x8fce(%rip) # a012 <encl_buffer.1.2> > mov 0xc(%rdi),%al > mov %al,0x8fd3(%rip) # a020 <encl_buffer.1.3> > mov 0xd(%rdi),%al > mov %al,0x8fce(%rip) # a024 <encl_buffer.1.4> > mov 0xe(%rdi),%al > mov %al,0x8fd1(%rip) # a030 <encl_buffer.1.5> > mov 0xf(%rdi),%al > mov %al,0x8fca(%rip) # a032 <encl_buffer.1.6> > ret > > Disassembly of section .data: > > 0000000000003000 <encl_buffer.0>: > 3000: 01 00 > ... > 0000000000004000 <encl_ssa_tcs1>: > > Thus, this proposed patch fixes both the size and location: > > 1. removing the static keyword from the encl_bug declaration ensures > that the _entire_ buffer is preserved with expected size, as the > compiler cannot anymore assume encl_buf is only used in this file. Could we use "used" attribute? https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html used This attribute, attached to a variable with static storage, means that the variable must be emitted even if it appears that the variable is not referenced. When applied to a static data member of a C++ class template, the attribute also means that the member is instantiated if the class itself is instantiated. > > 2. adding _attribute__((section(".data.encl_buffer"))) ensures that we > can control the expected location at the start of the .data section. I > think this is optional, as encl_buf always seems to be placed at the > start of .data in all my tests. But afaik this is not guaranteed as per > the C standard and such constraints on exact placement should better be > explicitly controlled in the linker script(?) This looks sane.
