This is the mail archive of the mailing list for the binutils project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] [RISCV] Support subtraction of .uleb128.

I tried the patch, and it works for me.  It even fixes one gcc
testsuite failure oddly enough, gcc.dg/debug/dwarf2/inline5.c, I
didn't look at the details of why.

I see that there is no actual relaxation of the uleb128.  We just pick
the size at assembly time via a variant frag, and then assuming that
the value can never increase during relaxation, we just need to add
enough leading zeros at link time to ensure that the size of the
uleb128 doesn't change if the value gets smaller.  It is a little
worrying that we can only support uleb128 and not sleb128 here, since
this trick won't work for sleb128, but I don't think that is a
problem.  Just handling uleb128 is good enough to allow gcc to use
uleb128/sleb128, as we only use subtraction on addresses, and they are
always treated as unsigned.  I see that object files get a bit larger
due to the extra relocations, but executables end up smaller because
the gcc_except_table is now smaller.  A simple rv32 C++ hello world
program linked against a static libstdc++ reduces in size by about
2.4% with this patch.

The assumption that the uleb128 value always decreases could be wrong
if we are subtracting a text section address from a data section
address, because the size of alignment padding between the two can
increase if the text section size decreases.  This code could fail in
this case.  I don't believe that gcc ever does this, but an end user
could conceivably write assembly code that does this which could be a
problem.  We could handle this in the assembler by forcing the uleb128
size to the pointer size if we are subtracting two pointers that
aren't in the same section.  I think that being in the same section is
enough, and that we don't have to require them to be in the same frag
because of how we handle alignment when relaxing.  That would make the
patch safer, and shouldn't change the size reduction we get with it.
This may require some inconvenient code duplication though, since you
are currently using the generic s_leb128 support which doesn't do
this.  Unless maybe we can add a target dependent hook there to avoid
copying it.

There is the issue I mentioned before that I'd like to see the psABI
updated before we modify binutils to use the new relocs.  Letting gcc
emit code that llvm can't link will cause problems.  Once this patch
goes in, gcc will automatically start emitting the new relocs because
a configure test that currently fails will start working.  I added
some info to the psABI issue to help, but I think we really need some
feedback from the LLVM folks before we commit this.

Otherwise, I don't see anything wrong with the patch other than a few
style issues.  These are all minor problems.

+       endp = p + len -1;
+       memset (p, 0x80, len);
+       *(endp) = 0;

Seem like the memset should be len - 1 but it seems harmless because we
just set the *endp value twice.

+  /* The length of unsigned-leb128 is variant, just assume the
+     size is one byte here.  */

I'd use "variable" instead of "variant", but that is just a style
issue.  This happens in two places.

+   Becuase relaxation may change the value of the subtraction, we
+   must resolve them in link-time.  */

Becuase -> Because
in -> at

+      /* Only unsigned leb128 can be handle.  */

handle -> handled

+      /* Insert relocations to resolve the subtraction in link-time.  */

in -> at


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]