[Libomptarget] Move ELF symbol extraction to the ELF utility #74717
Conversation
Summary: We shouldn't have the format specific ELF handling in the generic plugin manager. This patch moves that out of the implementation and into the ELF utilities. This patch changes the SHT_NOBITS case to be a hard error, which should be correct as the existing use already seemed to return an error if the result was a null pointer. This also uses a `const_cast`, which is bad practice. However, rebuilding the `constness` of all of this would be a massive overhaul, and this matches the previous behaviour (We would take a pointer to the image that is most likely read-only in the ELF).
| // A section with SHT_NOBITS occupies no space in the file and has no offset. | ||
| if (Section->sh_type == ELF::SHT_NOBITS) | ||
| return createError( | ||
| "invalid sh_type for symbol lookup, cannot be SHT_NOBITS"); |
There was a problem hiding this comment.
Why is this en error now? The ELF clearly has the symbol and for things like Size estimation we can use it just fine.
There was a problem hiding this comment.
I wasn't sure what the original intention was, as far as I could tell it would hit this error because there was a check for nullptr right after where we would look this up. That being said, even though we can check size and it exists, it still had no data associated with it so it seems kind of weird. If you want to look up a symbol you can use the other ELF Symbol lookup so that's my understanding.
There was a problem hiding this comment.
Now you can't call getMetadataFromImage anymore to get the size. I thought that could be useful. Anyway, we can add some new API or extend the other one if we need it.
There was a problem hiding this comment.
That's freely available with Sym->st_size. We could make a nicer looking helper if that's needed.
There was a problem hiding this comment.
Potentially we could make getMetadataFromImage do the nullptr handling. I just think that it's correct to have the function that gets the associated data to error if no such data exists.
Summary:
We shouldn't have the format specific ELF handling in the generic plugin
manager. This patch moves that out of the implementation and into the
ELF utilities. This patch changes the SHT_NOBITS case to be a hard
error, which should be correct as the existing use already seemed to
return an error if the result was a null pointer.
This also uses a
const_cast, which is bad practice. However,rebuilding the
constnessof all of this would be a massive overhaul,and this matches the previous behaviour (We would take a pointer to the
image that is most likely read-only in the ELF).