[PATCH] Implement the ldr-pseudo opcode for ARM assembly

David Peixotto dpeixott at codeaurora.org
Tue Nov 19 09:22:51 PST 2013


Hi Renato,

> Sorry it took so long, I'm looking at it now... Would be good to get the
> comments of as many people as we can, since this may be a sensitive issue
to
> some.

Sounds good to me.

> Some early comments:
> The ConstantPool object is creating as many labels as there are symbols in
the
> constant pool, will that get merged later? 

No, I see that the labels stick around. Initially I had created only one
label
per constant pool and made each reference as a MCExpr of Label+Offset. This
approach did not work for Thumb1, because it prevented the selection of the
16-bit ldr instruction needed for thumb1 because it failed to return true
for
the `isThumbMemPC()` function. I was not sure that it was safe to modify
that
function, so I used the current approach you see in the code. I think if I
used
MCContext::CreateTempSymbol() the labels would not stick around.

> Overall, looks good, but there is no check if the constant pools will be
> accessible from the instructions that use it, since they're just dumped at
the
> end of each section. That might work for most cases, but would be good to
at
> least add a FIXME somewhere.

I am relying on later code to detect this problem after relaxation and
fixups
are resolved. I do have one test for this in
test/MC/ARM/ldr-pseudo-obj-errors.s.

> I'll look at the test cases in more detail later, but it's good that there
are
> many of them. ;)

Please let me know if you see any areas for improvement or other tests that
should be added.


Thanks,
David





More information about the llvm-commits mailing list