This is an archive of the discontinued LLVM Phabricator instance.

[LLD][ELF][AArch64] Add missing LO12 relocation to usesOnlyLowPageBits
AbandonedPublic

Authored by peter.smith on Apr 30 2018, 1:51 AM.

Details

Summary

The R_AARCH64_TLSLE_ADD_TPREL_LO12_NC relocation also only uses the low page bits, but is missing from the usesOnlyLowPageBits function. At present the usesOnlyLowPageBits function is not called for this relocation but I think it is worth adding for the completeness of usesOnlyLowPageBits.

Diff Detail

Event Timeline

peter.smith created this revision.Apr 30 2018, 1:51 AM
grimar added a subscriber: grimar.May 3 2018, 5:33 AM

I would try to avoid dead code. So until it is possible to add a test case for this change, I would not do that probably.

Perhaps it is worth a comment instead as the function name implies a complete list. For example "// not a complete list, excludes relocations that can always be resolved statically such as TLS local exec."

grimar added a comment.May 3 2018, 6:34 AM

Perhaps it is worth a comment instead as the function name implies a complete list. For example "// not a complete list, excludes relocations that can always be resolved statically such as TLS local exec."

Yeah, maybe. I guess it already contains entries it never uses, btw?

Perhaps it is worth a comment instead as the function name implies a complete list. For example "// not a complete list, excludes relocations that can always be resolved statically such as TLS local exec."

Yeah, maybe. I guess it already contains entries it never uses, btw?

I think the ones that are already there can be used as they are one of R_GOT, R_PLT, R_TLSDESC, or (!AbsVal && !RelE) can be true. The local exec relocations are always AbsVal = true and all have a RelExpr of R_TLS.

I'm not hugely fussed either way, I was just unsure of whether to add new LO12_NC relocations to that function or not.

grimar added a comment.May 3 2018, 6:48 AM

Perhaps it is worth a comment instead as the function name implies a complete list. For example "// not a complete list, excludes relocations that can always be resolved statically such as TLS local exec."

Yeah, maybe. I guess it already contains entries it never uses, btw?

I think the ones that are already there can be used as they are one of R_GOT, R_PLT, R_TLSDESC, or (!AbsVal && !RelE) can be true. The local exec relocations are always AbsVal = true and all have a RelExpr of R_TLS.

I'm not hugely fussed either way, I was just unsure of whether to add new LO12_NC relocations to that function or not.

Ok. So, personally my opinion - I would not add any code that is unproven to be useful. This applies to this patch.

peter.smith abandoned this revision.May 3 2018, 6:54 AM

Ok I'll abandon this one and I'll update D46255 to remove the new TLSLE relocations I added to usesOnlyLowPageBits as the same argument applies.