This is an archive of the discontinued LLVM Phabricator instance.

[RuntimeDyld] Add accessors to `SectionEntry`; NFC
ClosedPublic

Authored by sanjoy on Nov 13 2015, 7:03 PM.

Details

Summary

Remove naked access to the data members in SectionEntry and route
accesses through accessor functions. This makes it obvious how the
instances of the class are used, and will also facilitate adding bounds
checking to advanceStubOffset in a later change.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 40197.Nov 13 2015, 7:03 PM
sanjoy retitled this revision from to [RuntimeDyld] Add accessors to `SectionEntry`; NFC.
sanjoy updated this object.
sanjoy added a subscriber: llvm-commits.

It seems like "Section.getAddress() + Offset" shows up pretty frequently. Do you think it's worth making a function to do that too?

Otherwise, this all looks pretty straightforward. All of my comments are about cleaning up things that exist in the current code.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp
866 ↗(On Diff #40197)

Is there a reason to keep the commented out debug code here? It should either be put inside a DEBUG() wrapper or deleted.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
442 ↗(On Diff #40197)

I know you didn't introduce them, but it would be nice to get rid of the C-style casts while you're making these changes.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h
293 ↗(On Diff #40197)

This cast is unnecessary.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.cpp
303 ↗(On Diff #40197)

These variables should be changed to be uint8_t* and the processFDE function should be updated similarly. I know it's equivalent, but it would be better to be consistent.

It seems like "Section.getAddress() + Offset" shows up pretty frequently. Do you think it's worth making a function to do that too?

Otherwise, this all looks pretty straightforward. All of my comments are about cleaning up things that exist in the current code.

Thanks for the review!

About getAddress -- what do you think of an interface like

template<typename T = uint8_t>
T* getAddress(size_t Offset = 0) {
  return static_cast<T *>(Address + Offset);
}

? That'll let us get rid of of the casts, and also add some bounds checking to getAddress in D14675.

It seems like "Section.getAddress() + Offset" shows up pretty frequently. Do you think it's worth making a function to do that too?

Otherwise, this all looks pretty straightforward. All of my comments are about cleaning up things that exist in the current code.

Thanks for the review!

About getAddress -- what do you think of an interface like

template<typename T = uint8_t>
T* getAddress(size_t Offset = 0) {
  return static_cast<T *>(Address + Offset);
}

? That'll let us get rid of of the casts, and also add some bounds checking to getAddress in D14675.

I'd prefer to see getAddressWithOffset(size_t Offset) as a separate function, but I do think it would be great to add bounds checking for this.

As for the template idea, you'd have to use reinterpret_cast to make the conversions used work, and I'd rather see the reinterpret_cast at the point of use so that it is obvious that such a heavy-handed cast is being used. Also, I think using a template in the offset case might make it unclear what's happening in a case like this (from the current code):

uint32_t *TargetPtr = reinterpret_cast<uint32_t *>(Section.getAddress() + Offset);

because this

uint32_t *TargetPtr = Section.getAddressWithOffset<uint32_t *>(Offset);

makes it ambigous as to whether the address is offset by Offset bytes or Offset uint32_t's.

I think this is easier to comprehend this way:

uint32_t *TargetPtr = reinterpret_cast<uint32_t*>(Section.getAddressWithOffset(Offset));

Even like that the function declaration will require a clear comment about what the offset parameter means.

sanjoy updated this revision to Diff 40846.Nov 20 2015, 4:35 PM
sanjoy marked 4 inline comments as done.
sanjoy edited edge metadata.

Address review

  • add getAddressWithOffset
  • add getLoadAddressWithOffset
  • other misc. review comments

This looks good, but there are still a bunch of places using the "([type])(getAddress() + Offset)" pattern.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
536 ↗(On Diff #40846)

The extra parens here and in the similar lines below are redundant now.

1252 ↗(On Diff #40846)

There are still some getAddress() + Offset cases here.

sanjoy updated this revision to Diff 40856.Nov 20 2015, 6:46 PM
sanjoy marked an inline comment as done.
  • address review: remove redundant parens & use getAddressWithOffset in more places
sanjoy added inline comments.Nov 20 2015, 6:48 PM
lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldCOFFX86_64.h
65 ↗(On Diff #40856)

Just noticed that there are some more of these left -- I'll upload a new diff fixing these shortly.

sanjoy updated this revision to Diff 40857.Nov 20 2015, 7:00 PM
  • address review: get the remaining getAddress() + Offset --> getAddressWithOffset(Offset) cases
sanjoy updated this revision to Diff 40858.Nov 20 2015, 7:30 PM
  • bugfix: re-associate the expression for COFF::IMAGE_REL_I386_DIR32NB. This shows up (as a failed assert) after adding the bound checking code in the next change

Just a few more cases that could be cleaned up.

lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp
1282 ↗(On Diff #40858)

C-style cast and could be getAddressWithOffset(Section.getStubOffset())

1298 ↗(On Diff #40858)

C-style cast

1314 ↗(On Diff #40858)

C-style cast and could be getAddressWithOffset(Section.getStubOffset()).

1481 ↗(On Diff #40858)

C-style cast and could use getAddressWithOffset

1525 ↗(On Diff #40858)

C-style cast

sanjoy updated this revision to Diff 40962.Nov 23 2015, 12:44 PM
sanjoy marked 5 inline comments as done.
  • address review
andrew.w.kaylor accepted this revision.Nov 23 2015, 1:23 PM
andrew.w.kaylor edited edge metadata.

Looks good.

Thanks for taking the time to work through all the small details!

This revision is now accepted and ready to land.Nov 23 2015, 1:23 PM
This revision was automatically updated to reflect the committed changes.