Re: [PATCH-H8300] Handling tiny_data attribute

[Jim Wilson <wilson at specifixinc dot com>]

On Tue, 2004-05-04 at 08:24, Grant Edwards wrote:
> A "tiny" read-only variable belongs in the low end of address
> space (0x0000-0x8000) in ROM with the other read-only stuff.  
> A "tiny" r/w variable belongs in the high end of address space
> (0xFF8000 - 0xFFFFFF) in RAM with the other r/w stuff.

OK.  I see what you are doing here.

> Additionally, the tiny r/w stuff in RAM (at 0xFF8000-0xFFFFFF)
> needs to be separated into initialized "data" and uninitialized
> "bss" sections so that startup code can deal with it
> appropriately -- they same way it does with the "non-tiny" data
> and bss sections.

I think tiny_data bss stuff will work as is.  This seems to be a space
optimization, to reduce the size of executable files.

> OK, I can add the sections to the linker scripts.  I still
> don't understand for which patform the default linker script is
> supposed to work, but I can add two lines to it without knowing
> that. :)

There isn't one default linker script.  There are 77.  It is only the
h8300 default linker script that we have to worry about, though there
are 7 default scripts provided for h8300.  This probably could be fixed
to expand them all from a template.  See binutils
src/ld/scripttempl/h8300*.

> Another option; we leave the old tiny_data attribute alone.
> Everything with that attribute goes into a read/write
> initialized data even if it's read-only or uninitialized.

That seems like a good solution.

> That will still place all three types of tiny variables (r/o,
> r/w-initialized, r/w-uninitialized) in a single section.  They 
> need to be in 3 separate sections:

That is OK, if you think of the tiny_data attribute as shorthand for
attribute section tiny.  When a user uses a section attribute, the data
is put in that section, even if it isn't the same kind of data as the
section would usually expect.  You can put r/o data in a r/w section,
and a program will work fine.  You only lose the ability to write
protect that data.  Likewise, you can put bss data in a r/w section, and
a program will work fine.  There is no need to require 3 separate
sections here.  You can get some useful behaviour if you have 3 separate
sections, but it is not required for a program to work.

Your suggestion of splitting tiny into 3 sections is a new feature that
provides additional optimization possibilities.  It is not a requirement
for this old tiny_data feature.

Another issue we should be concerned about is compatibility with other
targets.  There are other targets that have these kinds of attributes,
the v850 for instance, and it will be confusing for users if similar
attribute behave differently on different targets.

The h8300 tiny section conveniently splits into negative and positive
address ranges that cover ROM and RAM, but this isn't always true in
general.  The v850 for instance has a zero-page addressing mode, and
there is no way to split this into 3 sections in any meaningful way.

> Quite frankly, I see no point in the existing tiny_data
> attribute.  The compiler doesn't emit 16-bit addressing mode
> instructions so it's exactly the same as 
> __attribute__((section(".tiny_data")).

That isn't what I see.  For this

int foo __attribute ((tiny_data));
int bar;

int
sub (void)
{
  return foo + bar;
}

I get
	...
        mov.w   @_foo:16,r3
        mov.w   @_bar,r2
	...
which seems to clearly show that gcc is generating 16-bit addressing
modes for tiny_data variables.  Maybe this is accidentally broken in the
toolchain that you have?

> Can you suggest another architecture to look at that has
> similiar addressing modes?

I don't know if any other target has fixed this problem.  The code that
adds section flags is relatively new, and I don't think anybody has
considered the effect on embedded targets that have attributes that put
data in special sections, such as the h8300 tiny_data attribute.  The
v850 target though is a good example of a target that has a number of
similar attributes for different addressing modes.

> > I tried compiling some simple examples.  I don't see the need
> > for the tinybss section.  An uninitialized variables seems to
> > be handled correctly.
> I swear I tried it, and it goes into the "tinydata" initialized
> data section even though it's not an initialized varable.

This is OK, and correct.  This is what the tiny_data attribute is
supposed to do.  There is nothing wrong with placing uninitialized data
in a data section.  It will work.  If you didn't want it there, then you
shouldn't have added the tiny_data attribute.

> I wasn't aware of any "target platforms" except the generic
> "elf" one.  That one doesn't correspond to any real hardware,
> does it? Is the default linker script actually supposed to work
> for somebody? 

It works for the simulators.  It may have worked for one real board a
long time ago.  It is very common for people to have to write their own
linker scripts, as usually every embedded target board is different.

> Then you end up with all three types of tiny variables in a
> single section.  That's broken. They need to be in three
> separate sections, just like non-tiny ones do.

No.  This isn't broken.  This is the way that the tiny_data attribute is
supposed to work.  There is nothing wrong with this behaviour.  It just
isn't the behaviour that you want.  The behaviour you want is also
reasonable.

> The other thing I was trying to do (which hasn't been discussed
> yet) was fix it so that garbage collection works for tiny
> variables.  The -fdata-sections option (and hence garbage
> collection at link time) doesn't work for tiny variables in the
> existing implimentation.

My concern here is that if we have to add special -fdata-sections
support to the h8300 backend, then we also need to add it to every other
target port, in which it seems obvious that something in the gcc design
is broken, as we should just be able to add this code once in the
machine independent part of the compiler instead of adding it N times,
once in every target port.

> I think it a better idea to fix the tiny_data attribute so that
> it works properly

It isn't working properly, according to its specification.  It just
isn't working the way you want it to.

> The patch I wrote updated the documentation so that it
> described the modified behavior of the tiny_data attribute. Did
> the guys at KPIT lost that part?

I don't know.  Maybe I missed it or forgot about it.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com