This is an archive of the discontinued LLVM Phabricator instance.

[lld][ELF][ARM] Implement static (initial exec) TLS model
ClosedPublic

Authored by denis-protivensky on Mar 16 2015, 2:05 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

denis-protivensky retitled this revision from to [ELF][ARM] Implement static (initial exec) TLS model.
denis-protivensky updated this object.
denis-protivensky edited the test plan for this revision. (Show Details)
denis-protivensky added a project: lld.
denis-protivensky added a subscriber: Unknown Object (MLST).
denis-protivensky retitled this revision from [ELF][ARM] Implement static (initial exec) TLS model to [lld][ELF][ARM] Implement static (initial exec) TLS model.Mar 16 2015, 2:22 AM
ruiu added inline comments.Mar 16 2015, 11:24 AM
lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
325 ↗(On Diff #22014)

Off topic, but I always wonder if we want to sprinkle debug outputs like this in the ELF writers. They are longer than actual code and might be too verbose.

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
116 ↗(On Diff #22014)

Can you move the definition of ARMGotAtomContent here as a static local variable?

205 ↗(On Diff #22014)

Can ref.target() return a null pointer? If not, you want to use dyn_cast instead of dyn_cast_or_null.

lib/ReaderWriter/ELF/ARM/ARMTargetHandler.h
41 ↗(On Diff #22014)

If TP_TLS section is not found, it seems that this line will fail because tpOff has no value. Maybe you want to return a new _tpOff from the innermost block in the for loop and replace this line with llvm_unreachable?

Also you might want to use early return.

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
116 ↗(On Diff #22014)

Good.

205 ↗(On Diff #22014)

I think it shouldn't return null pointer. Some implementations use dyn_cast_or_null, this might have confused me. Will change to dyn_cast.

lib/ReaderWriter/ELF/ARM/ARMTargetHandler.h
41 ↗(On Diff #22014)

You're right that it's better to have llvm_unreachable than fail with unknown error. Will change.

lib/ReaderWriter/ELF/ARM/ARMRelocationHandler.cpp
325 ↗(On Diff #22014)

I occasionally read them to find values used for some related groups of relocs when I don't want to debug or see that this will be faster than debugging.

Updated:

  • reworked the code of tp_off calculation
  • replaced dyn_cast_or_null with dyn_cast

Didn't change atom content variables to be static class variables because it becomes inconsistent with other platforms, and there are more places to change, so the diff will grow for no reason. I can update it further in a separate commit if requested.

ruiu accepted this revision.Mar 17 2015, 12:02 PM
ruiu edited edge metadata.

LGTM

lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
207 ↗(On Diff #22082)

I guess early return is preferred. You can return std::error_code() here so that you can remove "else" below.

lib/ReaderWriter/ELF/ARM/ARMTargetHandler.h
37 ↗(On Diff #22082)

Can you add braces after this for?

This revision is now accepted and ready to land.Mar 17 2015, 12:02 PM
shankarke added inline comments.
lib/ReaderWriter/ELF/ARM/ARMRelocationPass.cpp
50 ↗(On Diff #22082)

only class declarations are allowed to be under anonymous namespace.

lib/ReaderWriter/ELF/ARM/ARMTargetHandler.h
33 ↗(On Diff #22082)

you can remove the const and remove mutable from _tpOff too.

denis-protivensky edited edge metadata.

Updated:

  • removed const and mutable in getTPOffset and made style cleanup
  • made global variables static and out of anonymous namespace
  • use early return in handleTLSIE32
lib/ReaderWriter/ELF/ARM/ARMTargetHandler.h
33 ↗(On Diff #22082)

I will do that, but I think lazy initialization is the case where using mutables does make sense.

shankar.easwaran edited edge metadata.

LGTM

This revision was automatically updated to reflect the committed changes.