This is an archive of the discontinued LLVM Phabricator instance.

[MC] Support .reloc sym+constant, *, *
ClosedPublic

Authored by MaskRay on Jul 14 2020, 12:35 AM.

Details

Summary

For .reloc offset, *, *, currently offset can be a constant or symbol.
This patch makes it support any expression which can be folded to sym+constant.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 14 2020, 12:35 AM
MaskRay updated this revision to Diff 277692.Jul 14 2020, 12:42 AM

Improve tests

MaskRay updated this revision to Diff 277694.Jul 14 2020, 12:45 AM

Move comment to the base emitRelocDirective.

Thank you for putting up a patch in response to your comment.
Once this goes in I'll rebase D79625 on top of it and that patch should become a lot smaller.

llvm/include/llvm/MC/MCStreamer.h
1025

I feel like you have changed the meaning of this default behaviour. Was that the intention?
Before we used to return true which indicated that the reloc could not be emitted. Now you return None meaning it succeeded.

llvm/lib/MC/MCObjectStreamer.cpp
688

I assume this is what you meant by simpler code. We already have a function to do this computation. :)

704

One of the issues I noticed with this situation is that the symbol may not be in the same data fragment as the .reloc directive. The code you have assumes that it is in the same DF.

I used the same DF as the symbol:

MCFragment *Fragment = Symbol->getSymbol().getFragment();
if (!Fragment || Fragment->getKind() != MCFragment::FT_Data)
  return false;
stefanp added inline comments.Jul 14 2020, 9:11 AM
llvm/test/MC/ELF/reloc-directive.s
24

One more comment. Since this is sym+const we should have a test along the lines of:

.globl a
	​a: ret
. reloc a+4, R_X86_64_NONE, foo
MaskRay updated this revision to Diff 277872.Jul 14 2020, 9:30 AM
MaskRay marked 5 inline comments as done.

Improve tests

llvm/include/llvm/MC/MCStreamer.h
1025

This is intentional. parseDirectiveReloc tried doing things duplicated with emitRelocDirective (evaluate absolute). By moving logic to emitRelocDirective, we have to let emitRelocDirective handle diagnostics.

llvm/lib/MC/MCObjectStreamer.cpp
704

GNU as somehow allows the following (it behaves as if .reloc is placed in .data)

.text
.reloc .data, R_X86_64_NONE, 0

.data
.globl 
data:

It needs thoughts how to handle/detect this. This probably can go to a separate change.

stefanp added inline comments.Jul 14 2020, 11:21 AM
llvm/lib/MC/MCObjectStreamer.cpp
704

I wouldn't know where to start with that .data reloc because I don't think we would be able to get the DF for it. I almost feel like we would have to make changes to ELFObjectWriter::recordRelocation and at least allow MCFixup::create to accept more than uint32_t as the offset. But that is a lot of work. Definitely a different patch.

Either way, I don't need the .data but I will need to cover the situation where the DF is different for the symbol. This is for code that will be coming from me later on. For this patch if you want to leave it as a TODO I'm ok with that. I can try to address it in the rework of D79625.

MaskRay marked an inline comment as done.Jul 14 2020, 12:31 PM
MaskRay added inline comments.
llvm/lib/MC/MCObjectStreamer.cpp
704

I'll leave it as a TODO. This did not work correctly before. The intention of the patch is just to add offset support, not thinking hard about how a foreign MCDataFragment works...

Do you have more review comments:) ?

MaskRay marked an inline comment as done.Jul 14 2020, 12:46 PM
MaskRay added inline comments.
llvm/lib/MC/MCObjectStreamer.cpp
704

Actually I have added a FIXME :)

stefanp accepted this revision as: stefanp.Jul 14 2020, 1:18 PM

Nope, no more comments.
LGTM

This revision is now accepted and ready to land.Jul 14 2020, 1:18 PM
MaskRay updated this revision to Diff 277963.Jul 14 2020, 1:44 PM

Merge reloc-directive-err.s into reloc-directive.s with --defsym=ERR=1

This revision was automatically updated to reflect the committed changes.