[PATCH] Add RelocVisitor support for MachO
kfischer at college.harvard.edu
Tue Mar 17 14:19:26 PDT 2015
> I’m sorry, I’m out of work for a few weeks, and I won’t be able to review
> it correctly. However a couple of things stood out when I looked at it last
No problem, thanks for the reply - my apologies if my reply sounded harsh,
I've just been carrying some version of this patch locally for about a year
and carrying LLVM patches is a pain, even if they're small.
> - you definitely should separate the debug line fix from the RelocVisitor
Ok, but the problem is that I'm not sure I can cook up a test case in ELF
that will properly demonstrate this problem. Similarly, the test case for
MachO fails without the bugfix. I suppose we can get the MachO RelocVisitor
implementation in with an incorrect test case and then have the bugfix
change that test case. Does that sound reasonable?
> - MachO object files contain another relocation type
> (X86_64_RELOC_SUBTRACTOR if memory serves), and I don’t know if it really
> fits in the RelocVisitor model (as it needs access to the following reloc
> to make sense). Why is RELOC_UNSIGNED sufficient for your usecase?
I haven't had LLVM generate anything else on my test cases, so I didn't
need to implement it. I think it's easier to implement them one at a time
with proper test cases, which I didn't have for the X86_64_RELOC_SUBTRACTOR
case. I'd be happy to implement the other relocations, but I think it make
sense to get the simple case settled first.
> > REPOSITORY
> > rL LLVM
> > http://reviews.llvm.org/D8148
> > EMAIL PREFERENCES
> > http://reviews.llvm.org/settings/panel/emailpreferences/
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the llvm-commits