On Fri, Oct 28, 2016 at 09:42:10PM -0600, Jeff Law wrote:
>
> Consider this definition of ASM_GENERATE_INTERNAL_LABEL (from sp64-elf.h):
>
> #undef ASM_GENERATE_INTERNAL_LABEL
> #define ASM_GENERATE_INTERNAL_LABEL(LABEL,PREFIX,NUM) \
> sprintf ((LABEL), "*.L%s%ld", (PREFIX), (long)(NUM))
>
> And a use from assemble_static_space:
>
> ASM_GENERATE_INTERNAL_LABEL (name, "LF", const_labelno);
>
>
> For this case we can generate up to 16 bytes of data + a nul terminator.
>
> Sadly, we only allocate 16 bytes in assemble_static_space.
>
> Obviously it's unlikely we'll ever have a labelno that will overflow to
> -2147483648 without something else breaking. So in practice this isn't
> likely to ever cause a problem. But we still need to address it.
>
> This causes 8 sparc configurations from config-list.mk to fail to build
> using the trunk compiler to build the crosses.
>
>
>
> We can obviously fix the array to be bigger here and it's a trivial change.
> If we get a situation where it's out of range again, we can detect it with
> the existing sprintf warnings. It's also consistent in the sense that most
> callers to ASM_GENERATE_INTERNAL_LABEL use a significantly larger buffer
> than assemble_static_space.
>
> Sadly, there's a bigger issue here. Namely that the caller and the
> definition of ASM_GENERATE_INTERNAL_LABEL both can include arbitrary length
> text into the label name. Furthermore, the buffer is allocated in the
> caller's context. It's a terrible API.
>
> ISTM the way "out" is to change very ASM_GENERATE_INTERNAL_LABEL
> implementation to use snprintf to first determine the length of the
> resulting string, then allocate an appropriate amount of memory (returning
> it to the caller). The caller is then changed to use the buffer allocated
> by ASM_GENERATE_INTERNAL_LABEL, free-ing it when appropriate. Major ick.
> We'd probably want to hook-ize the damn thing while we're at it.
>
> Other thoughts?
So actually a thing I've wanted to do for a while is add a long string
building class to generate asm into that would use a set of buffers
adding new ones when the space is needed. So that would look something
like
class string_builder
{
public:
string_builder () {}
~string_builder ()
{
for (unsigned i = 0; i < m_buffers.length (); i++)
free (m_buffers[i]);
}
void append_string (const char *s)
{
unsigned int length = strlen (s);
if (length + m_last_buf_length >= MAX_BUF_LENGTH)
alloc_buffer ();
strcat (m_buffers.last (), s);
m_last_buf_length += length;
}
void append_printf (const char *s...)
{
// something somewhat more complicated than apend_string but similar
// in spirit
}
void write (FILE *f)
{
for (unsigned int i = 0; i < m_buffers.length (); i++)
fwrite (m_buffers[i], 1, MAX_BUFFER_LENGTH, f);
}
private:
void alloc_buffer ()
{
m_buffers.safe_push (malloc (MAX_BUFFER_LENGTH));
m_buffers.last ()[0] = 0;
m_last_buf_length = 0;
}
auto_vec<char *> m_buffers;
size_t m_last_buf_length;
};
The original reason was to reduce calls into libc while writing out
assembly to speed that up somewhat, but it should also work here so the
caller doesn't need to care about the buffer size. We might need to add
a way to get the string out as one piece.
Although actually it seems likely we'll always need the string as one
piece here, and it will usually be small, so maybe we're better served
here with a standard C++ string of some sort.
Unfortunately I haven't found time yet to work on either the
string_builder or a plain string for gcc yet, sorry.
Trev
p.s. I think there are some non optimal and buggy things in the example,
but you get the picture.
>
> Jeff
