[PATCH] Add support for subsections to the ELF assembler. Fixes PR8717.

Rafael EspĂ­ndola rafael.espindola at gmail.com
Wed Apr 10 05:49:07 PDT 2013


On 10 April 2013 02:47, Peter Collingbourne <peter at pcc.me.uk> wrote:
>
>   > Please add a test showing this.
>
>   To be clear, I'll add a test making sure we don't emit a relocation in this case.

Please also test that we use a 2 byte jump.

>   > Please make sure we reject numbers larger than 8192.
>
>   I thought about this, but a seemingly undocumented feature of gas is that it supports larger numbers, as well as negative numbers, so I decided to omit the check (although it's debatable whether we need to be compatible on this).  If we decide not to support this we can probably simplify getSubsectionInsertionPoint as well.

Do you depend on that feature? If not it is probably best to start
with the more restrictive option.

> It seems better to me to be able to deal with the insertion logic in this function rather than having to repetitively deal with insertion throughout the MCObjectStreamers.  In my opinion, repetition would make it easier to make a mistake in one of the streamers and (e.g.) have fragments appended to the fragment list rather than inserted in the right place.

Why would it be repeated? Instead of

Fragment *F = new...;
insert(F);

you get

Frament F = new Fragment(....CurInsertionPoint);

and the compiler makes sure CurInsertionPoint is always passed.

>> Can we report the error without using report_fatal_error?
> Yes, but this would be a relatively invasive QOI improvement to handle a relatively rare occurrence; it would probably entail changing ChangeSection and SwitchSection to return an error value, and SwitchSection has a large number of callers that would each need to be audited to make sure they're handling errors correctly.

OK. We can do this in another patch then.

Thanks,
Rafael




More information about the llvm-commits mailing list