RFD: Buffer handling for ASM_GENERATE_INTERNAL_LABEL

RFD: Buffer handling for ASM_GENERATE_INTERNAL_LABEL

[Jeff Law]

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?

Jeff

Re: RFD: Buffer handling for ASM_GENERATE_INTERNAL_LABEL

[Trevor Saunders]

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

Re: RFD: Buffer handling for ASM_GENERATE_INTERNAL_LABEL

[Bernd Schmidt]

On 10/29/2016 04:17 PM, Trevor Saunders wrote:
>
> 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

I'd do something a little simpler and just wrap an obstack in a class.


Bernd

Re: RFD: Buffer handling for ASM_GENERATE_INTERNAL_LABEL

[Bernd Edlinger]

Hi,


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

Yes.  Even ASM_FORMAT_PRIVATE_NAME looks like a better way to define
the API.

# define ASM_FORMAT_PRIVATE_NAME(OUTPUT, NAME, LABELNO) \
   do { const char *const name_ = (NAME); \
        char *const output_ = (OUTPUT) = \
          (char *) alloca (strlen (name_) + 32); \
        sprintf (output_, ASM_PN_FORMAT, name_, (unsigned long)(LABELNO)); \
   } while (0)


But it also depends on what ASM_PN_FORMAT looks like,
so also this "+ 32" should be a define, which tells us how much
overhead comes from the format string, then we should use that
in the allocation.

If the target overrides ASM_PN_FORMAT the target should also
define how long the ASM_PN_FORMAT prints minimum.  Then the macro
can use strlen (name_) + ASM_PN_FORMAT_MIN_LENGH + max. space
for the LABELNO, maybe just use strlen(ASM_PN_FORMAT) if the
target fails to cooperate.

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


I would like to have a simple solution, that does work with the current
code, without totally rewriting the users.

The middle end should be able to tell the target how long the longest
PREFIX will be, then the target should be able to give a formula how
long the total format will be.  Then some users already use
MAX_ARTIFICIAL_LABEL_BYTES to allocate a sufficient buffer, that should
be based on the target definition, all others that don't do it already
should follow.


Bernd.