Re: Failures in branch range calculation

[Caroline Tice <ctice at apple dot com>]

I sent the following response to Richard Earnshaw, but forgot to CC the 
gcc-patches list.  Sorry.

-- Caroline Tice
ctice@apple


I have a patch I am currently working on for updating/fixing a lot of 
small problems with
the hot/cold partitioning stuff.  Currently, in my patch, the code for 
the function
text_section looks like this:

void
text_section (void)
{
  if (in_section != in_text)
    {
      in_section = in_text;
      fprintf (asm_out_file, "%s\n", TEXT_SECTION_ASM_OP);
      assemble_align (FUNCTION_BOUNDARY);
    }
}

Will this be okay?  Will it fix your problem?  If not, what do you 
think would be better?  I do
not know when I will be able to submit my full patch, because I am 
running into some
very obscure, difficult-to-track-down bugs when I try to do a profiled 
bootstrap on an x86/Linux
machine, doing partitioning.  But I would be happy to submit (or have 
you submit) a patch
containing the above fix, if you think that would be appropriate.  
Please let me know what
you think.

-- Caroline Tice
ctice@apple.com

On May 22, 2004, at 5:08 AM, Richard Earnshaw wrote:

> I've been trying to track down the reason why the compiler has started
> occasionally generating out-of-range branches on the thumb target.  It
> turns out to be as a result of the hot-and-cold code regions changes.
>
> One of the changes in that patch was to make text_section() add an
> unconditional alignment.  This started off as:
>
> > > > + #define SECTION_FORMAT_STRING".section\t\"%s\"\n\t.align 2\n"
> > >
> > > 	What is this for?
> >
> > To make sure the correct alignments, "dots", etc. end up being
> > output for whichever architecture
> > we are on, when writing out the assembly directive for
> > changing between hot/cold sections.  Yes, this
> > *is* necessary.  Without it I get bugs on both architectures
> > I've tried this on.
>
> and was subsequently changed to be an explicit alignment statement in
> text_section() by Andrew.
>
> This causes a problem on Thumb because we output switch tables in the
> read-only data section and then switch back to the text section and get
> an unaccounted alignment statement in the output stream.  Thumb
> instructions are only 2 bytes long, so a 4-byte alignment at this point
> is bad news.
>
> The change to add an unconditional alignment is just wrong.
> Conceptually, and in practice.
>
> - Just because we've switched to a new section doesn't automatically
> imply that things in it need aligning.
>
> - It's really wrong that the alignment is a manifest constant...
>
> - And it's really really wrong that branch range calculation doesn't
> take this into account.
>
> Almost invariably if we've just switched to the text section (and we
> weren't there previously) the the next thing that will happen is that a
> label will be emitted.  The alignment belongs on that label not on the
> section change.
>
> Whatever, the alignment doesn't belong here, so I really think the only
> correct patch here is to back this bit out.
>
> However, I don't know what the particular problem was that was
> supposedly solved by this change, so it's impossible for me to evaluate
> exactly what it was intended to fix and what the most appropriate fix 
> is
> elsewhere.
>
> I'll give this a little time (but not too much) for discussion...
>
> R.