[PATCH-H8300] Handling tiny_data attribute

[Grant Edwards]

> You must be looking at older gcc sources.  Current sources are
> written in ISO C, and use of prototypes is required.  We made
> this change over the last year, so 3.3 is old-style, but 3.4
> is new-style.

Yes. My patch was against 3.3.  I work pretty much exclusively
with the releases from KPIT-Cummins.

>>> Changing .tiny sections support won't work without appropriate
>>> binutils support.
>
>> I'm using it without any binutils changes and it works just
>> fine.
> 
> Your patch creates new sections that did not exist before.

Right.  That was the goal.

The problem I'm trying to fix is that the existing solution
places all "tiny" viarables in a single r/w initialized data
section regardless of whether they're initialized or not and
regardless of whether they're read-write or read-only.

This is just plain broken behavior.  There are very good
reasons normal variables are split up into rodata, data, and
bss. Tiny variables need to be split up the same way, for
exactly the same reasons.

> If these sections are not in the linker script (and they are
> not in the standard linker scripts in binutils), then the
> linker will use default rules to place them.  Default rules
> use the section flags, so tinybss gets put next to bss, and
> tinyrodata gets put next to rodata.  This means neither
> tinybss nor tinyrodata is placed in the tiny data segment,

But we don't want tinyrodata placed in the tinydata segment.
That's the basic problem I'm trying to fix.  The attribute
conflict error message is just a symptom of the broken
behavior.  Eliminating the error message won't help if the r/o
and r/w data are still mixed together in a single section.

The tinyrodata needs to go someplace completely different:
right between the interrupt vectors and the "normal" text and
rodata sections (generally -- but it depends on the design of
the target hardware). 

Perhaps I should digress a bit...

On an H8 board (typically), read-only stuff goes in ROM at the
low end of address space and read/write stuff goes in RAM at
the high end.

Each end of the address space contains a 32KB region that can
be addressed using a 16-bit addressing mode rather than a
24-bit addressing mode.

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.

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.

[OT: I think the word "tiny" was an unfortunate choice, since
there's an even smaller 8-bit addressing mode available.  I
always think of the 8-bit addressing mode as "tiny", the 16-bit
addressing mode as "small", and the 24 bit one as "large", but
that's just me.]

> and hence code that requires this will fail.  You might not
> notice a problem if your program is small, but I would expect
> this to fail for a large enough program.  I haven't done a
> toolchain build to verify this, but I don't see how this can
> possibly work.

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. :)

> We should consider whether the pain of asking everyone to fix
> their linker scripts is worthwhile.  Most people aren't going
> to know that they need to fix their linker scripts to continue
> using the tiny_data attribute.  Their programs will fail in
> strange ways, they will file bug reports to us, and then we
> will have to explain.  I would prefer a solution that doesn't
> require us to answer bug reports about this.

That's understandable.

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.

I can create a single, new attribute that automatically places
the variable in a data, rodata, or bss section based on the
attributes of the variable. That will require a bit more work
than fixing the existing attribute, but it will preserve the
old, broken behavior for those who rely on it.

I don't think adding two lines to a linker script is much to
ask of users, but they may disagree.

> It might be simpler to fix gcc so that these errors don't
> happen.  I suggested a solution before, which is to force the
> section flags to be what we want, "aw".

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:

The tiny read-only data needs to go at the low end of the
address space, so it needs to be in a section separate from the
tiny read-write data.

The tiny read-write data needs to go at the high end of the
address space, but startup code needs to be able to handle
intitized vs. uninitialized tiny data properly, so the two
types of read-write tiny data need to be in separate sections.

> Another alternative is to reject variables that require wrong
> section flags, e.g. if the tiny_data attribute is applied to a
> static const, then give an error instead of creating a section
> with the wrong section flags.  Or give a warning and ignore
> the const attribute.  Or maybe create a new tiny_rodata
> attribute, which maps to the new section.  That way only
> people using the new attribute need to worry about fixing
> their linker scripts.

We'd also need to create a new tiny_bss attribute as well.  I
prefer to let the compiler figure out whether it's rodata,
data, or bss.  I'm lazy that way: I expect the compiler to
figure that sort of stuff out for me. Besides, if you go the
explicit, brute force route, its just a different spelling of
__attribute__((section("whatever")).

I see no benefit of
      __attribute__((tinybss))
      __attribute__((tinydata))
      __attribute__((tinyrodata))
over 
      __attribute__((section(".mytinybss"))
      __attribute__((section(".mytinydata"))
      __attribute__((section(".mytinyrodata"))

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")).
      
> It might be useful to look at other ports and see if any
> others have already run into this problem, and look at how
> they solved it.

Can you suggest another architecture to look at that has
similiar 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.

> > BTW, what target platform is the default linker script for?
> 
> Every target platform has its own default linker script.

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? I've been using gcc for embedded development for
many years, and didn't know that the default linker scripts
were supposed to be work.  I've never run into anybody using
them...

>> >It might be easier to provide default section flags for the
>> >tiny section rather than letting gcc compute them.
>
>> Sorry, I don't know what that means.
> 
> Gcc is automatically computing section flags (the "aw" or "a")
> based on the data that is put into the section.  This happens
> in default_section_type_flags, which is called from the
> TARGET_SECTION_TYPE_FLAGS hook.  So a possibly solution is to
> define this hook for the h8300 port, and force the tiny
> section to have "aw" section flags.

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.

> > The whole point of the exercise was to create three different
> > sections, so I don't really understand what you're suggesting.
> > Will "default section flags" provide a way for "tiny" variables
> > to be placed into three separate sections?
> 
> No.  It avoids the need to create 3 separate sections.

Creating three separate sections is what I'm trying to do. That
need is created by the very architecture design of the H8/300
(well the need for two of them anyway -- the third is just
common sense).

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 patch also fixes that.

> 3 separate sections is bad because it will break the compiler
> for any user who doesn't modify their linker script.  It is
> generally not a good idea to change an existing feature in
> this way.  If you really need 3 sections, then it is probably
> better to create a new feature than to change an old one.

OK.  I've never added an attribute, so I'll start looking at
that.

> You could maybe create two new attributes that do what you
> want instead of changing the meaning of the existing
> attribute.  It isn't clear to me why you need 3 different
> sections though.

Hopfully I've explained it in this message.  If not, shout and
I'll try again.

> The only reason I was given was to avoid section flag
> conflicts, and there are easier ways to solve that problem.

I don't care about the flag conflicts.  That can obviously be
eliminated by deleting the 'const' storage class, but the
underlying problem still exists.

The goal is to split tiny variables up into tiny versions of
the rodata, data, and bss sections -- complete with support for
-fdata-sections and hence garbage collection by the linker.

I want the 'const' modifier to place the variable into a
read-only section so I can put it in a read-only section of
memory space.  I also want data and bss separated for the same
reason they were separated 35 years ago: so that startup code
can zero out bss using a loop rather than having a whole
section in ROM of initialization data full of zeros.

> Your patch breaks backwards ABI compatibility, which is
> usually a bad thing.

Agreed.

> If you can get major h8300 users to agree that this is
> necessary, then you can go forward. However, since I am not a
> h8300 maintainer, and I don't know who the major users are, I
> am not comfortable approving changes that break the ABI.

Fair enough.  If people want to keep the existing behavior, I
will work on a single new attribute that will cause the
variable to be automatically placed into one of three new
sections (or sets of sections if -fdata-sections is enabled).

I think it a better idea to fix the tiny_data attribute so that
it works properly and ask for people to add two lines to their
linker scripts for the two new sections, but if people don't
want to add the two lines to their linker scripts to handle the
tinybss and tinyrodata sections, I'm willing to leave the
existing attribute alone and add a new one.

> Another issue, the tiny_data attribute is documented in the
> gcc manual.  If we are changing the behaviour of it, then we
> also need to change the documentation.  Search in
> gcc/doc/extend.texi for tiny_data.

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?

-- 
Grant Edwards
grante@visi.com