[PATCH] D62838: [Object] add isExecutableObject member function

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 4 inline comments as done.
abrachet added inline comments.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:1203
+template <class ELFT> bool ELFObjectFile<ELFT>::isExecutableObject() const {
+  return EF.getHeader()->e_type == ELF::ET_EXEC;
+}
----------------
MaskRay wrote:
> Some people will be surprised if `ET_DYN` is not considered as executable object.
> 
> Position-Independent Executables use `ET_DYN`.
$ llvm-readelf `which llvm-readelf` -h
...
Type:                              DYN (Shared object file)

Oh yeah, good catch, thank you. How would you write this method? Does an executable always have e_entry as non zero? Is there ever a case where execve(2) will not fail if e_entry is 0? 

Also, should I worry about this? https://github.com/freebsd/freebsd/blob/master/sys/kern/imgact_elf.c#L1135 I don't know enough about ELF yet on this one. Thanks.


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:47
+class ELFTest : public TestBase<ELF64LE::Ehdr> {
+  void createGenericHeader() override {
+    memset(&Header, 0, sizeof(Header));
----------------
jhenderson wrote:
> You've marked this as `private` (implicitly) here, but it's `protected` in the base class. Is that intended?
Well createGenericHeader is only ever called by SetUp in TestBase, it isn't necessary to be called outside of the class.


================
Comment at: llvm/unittests/Object/ObjectFileTest.cpp:152
+
+TEST_F(MachOTest, IsRelocatable) {
+  for (auto Type : Types) {
----------------
jhenderson wrote:
> I wonder if it would make more sense to fold this into the test case above? i.e. the check becomes:
> 
> ```
>     if (Type == MachO::MH_EXECUTE) {
>       EXPECT_TRUE(ObjPtr->isExecutableObject());
>       EXPECT_FALSE(ObjPtr->isRelocatableObject());
>     } else if (Type == MachO::MH_OBJECT) {
>       EXPECT_FALSE(ObjPtr->isExecutableObject());
>       EXPECT_TRUE(ObjPtr->isRelocatableObject());
>     } else { // Don't know if this is really necessary or not, but you get the idea
>       EXPECT_FALSE(ObjPtr->isExecutableObject());
>       EXPECT_FALSE(ObjPtr->isRelocatableObject());
>     }
> ```
> 
> On the other hand, if you think the separate test-cases is easier to read, I'm okay with it.
> 
> Same comment applies to ELF.
Hmm, I think that the current way is better for when a failure occurs because the output will be more clear on which test it failed. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62838/new/

https://reviews.llvm.org/D62838