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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added inline comments.


================
Comment at: llvm/unittests/Object/CMakeLists.txt:10
   SymbolicFileTest.cpp
+  IsExecutableTest.cpp
   )
----------------
nit: I'd put this in a file called `ELFObjectFileTest.cpp`. Usually unit test files have a 1:1 correspondence with the file they're testing, and individual test cases for the methods/scenarios they're testing.


================
Comment at: llvm/unittests/Object/IsExecutableTest.cpp:31
+    memcpy(&Header.e_ident, e_ident, 16);
+    Header.e_type = ELF::ET_REL;
+    Header.e_machine = ELF::EM_X86_64;
----------------
It'd be good to parameterize (i.e. with a loop) this on other ELF types too: ET_NONE, ET_DYN, ET_CORE, and maybe a few non-enum types (pick an arbitrary number between ET_LOPROC and ET_HIPROC).

Note: within a loop, you can use stream logging w/ ASSERT_*/EXPECT_* macros to print the type value if the assertion fails (since gtest won't print it otherwise), e.g.
```
for (...) {
  Header.e_type = ElfType;
  ...
  EXPECT_FALSE(ObjFileOrErr.get()->isExecutableObject()) << " for " << ElfType;
}


================
Comment at: llvm/unittests/Object/IsExecutableTest.cpp:38-39
+    auto ObjFileOrErr = ObjectFile::createObjectFile(MemoryBufferRef(Bytes, ""));
+    ASSERT_TRUE(!!ObjFileOrErr);
+    ASSERT_FALSE(ObjFileOrErr.get()->isExecutableObject());
+
----------------
For ASSERT (which aborts the test case) vs EXPECT (which causes failure but continues with the test) in gtest, you usually want to use ASSERT only when the condition being false will cause fatal errors later, and EXPECT when the test can continue to check other things.

`ASSERT_TRUE(!!ObjFileOrErr)` is a good check because calling `ObjFileOrErr.get()->isExecutableObject()` on the next line would (probably) cause the test to crash anyway (I think `ObjFileOrErr.get()` would be `nullptr` in that case?)
However, `ASSERT_FALSE(ObjFileOrErr.get()->isExecutableObject())` should be `EXPECT_FALSE` instead -- it's not a fatal error, and you might want to see what the result is when you change the type to ET_EXEC. (e.g. maybe that will also fail because the check is backwards, or maybe it will pass because `isExecutableObject()` always returns true, etc.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62838