[PATCH] D53051: [llvm-tapi] initial commit, supports ELF text stubs
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 1 03:29:19 PDT 2018
jhenderson added a comment.
It would be good to see some doxygen comments in the header files.
================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:10-11
+//
+// This file defines an internal representation of an ELF stub as well as an
+// interface for interacting with ELF files.
+//
----------------
According to https://llvm.org/docs/CodingStandards.html#file-headers, this should be doxygen-style.
================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:30
+
+ // bits 0 - 10 reserved for XCode
+
----------------
Here and elsewhere, comments should start with a capital, and end with a full-stop.
================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:37-38
+ ELF_64BE = 1u << 14,
+ // Text-based ELF stub file (.tbe) version 1.0
+ TBE_V1 = 1u << 15,
+ All = ~0u,
----------------
Maybe drop the V1/version 1.0 here?
================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:43-46
+ NoType = ELF::STT_NOTYPE,
+ Object = ELF::STT_OBJECT,
+ Func = ELF::STT_FUNC,
+ TLS = ELF::STT_TLS,
----------------
This hasn't been clang-formatted.
================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:48
+
+ // Type information is a nibble, so 16 is safely out of range.
+ Unknown = 16,
----------------
I'm not sure I understand what "nibble" means.
================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:53
+struct ELFSymbol {
+ std::string StName;
+ unsigned int StSize;
----------------
I'm not sure there's any benefit prefixing these names with St. They are already scoped to a symbol, and most don't even directly map to an ELF symbol member (st_name is an index not a name, for example, while StUndefined and StWarning have no direct equivalent).
================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:54
+ std::string StName;
+ unsigned int StSize;
+ ELFSymbolType StType;
----------------
This should be uint64_t (ELF64 uses 64-bit symbol sizes).
================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:71
+ explicit ELFStubFile(FileType Type) {
+ this->Type = Type;
+ }
----------------
Here and below, do this in the initialisation list:
```
explicit ELFStubFile(FileType Type) : Type(Type) {}
```
================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:73
+ }
+ explicit ELFStubFile(FileType Type, ELFStub &SourceStub) {
+ this->Type = Type;
----------------
This should probably go into a .cpp, given its relative complexity.
================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:76
+
+ Stub.Symbols.clear();
+ Stub.Arch = SourceStub.Arch;
----------------
I'm not sure that there's any benefit to this line here - the vector will be empty already.
================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:77-82
+ Stub.Arch = SourceStub.Arch;
+ Stub.SoName = SourceStub.SoName;
+ for (auto Elem : SourceStub.Symbols) {
+ Stub.Symbols.push_back(Elem);
+ }
+ llvm::sort(Stub.Symbols, symbolCompareTo);
----------------
I'm not sure about this, but maybe this should all go in a constructor for ELFStub?
================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:100
+ // Sets this ELFStubFile's type to a supported format.
+ Error setFileType(FileType NewType) {
+ if (supportsType(NewType)) {
----------------
If you moved the definition into a .cpp, you could remove the dependency on Error.h from this header file.
================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:105
+ } else {
+ return make_error<StringError>(
+ "Invalid file type for ELF stub.",
----------------
You can use createStringError to simplify this slightly. I'm not sure, but perhaps you should include the file type received in the error message?
================
Comment at: llvm/include/llvm/TAPI/TBEHandler.h:17-20
+#include "llvm/Support/Error.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/YAMLTraits.h"
+#include "llvm/TAPI/ELFStubFile.h"
----------------
Most (all?) of these includes can be replaced with forward-declarations.
================
Comment at: llvm/lib/TAPI/TBEHandler.cpp:37
+ llvm::raw_ostream &Out) {
+ static const std::map<uint16_t, std::string> ArchMap = {
+ {(uint16_t) ELF::EM_X86_64, "x86_64"},
----------------
By my understanding, this should take StringRef, as we are dealing with string literals.
I'm also wondering whether this is the wrong kind of map to use, but I'm not familiar enough with LLVM's various map options to propose an alternative.
================
Comment at: llvm/lib/TAPI/TBEHandler.cpp:49
+ static StringRef input(StringRef Scalar, void*, ELFArchMapper &Value) {
+ static const std::map<std::string, uint16_t> ArchMap = {
+ {"x86_64", (uint16_t) ELF::EM_X86_64},
----------------
Same comments as above.
================
Comment at: llvm/lib/TAPI/TBEHandler.cpp:134
+ if (File == nullptr) {
+ return make_error<StringError>("Can't write null file.",
+ std::make_error_code(std::errc::invalid_argument));
----------------
You can use createStringError here too. I'm wondering whether it should just be an assertion though, not sure about that.
================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:25
+
+ auto File = Handler.readFile(Buf);
+ EXPECT_NE(File.get(), nullptr);
----------------
Too much auto maybe (here and elsewhere in this file)? I'm not sure what the type of File is here.
================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:37-40
+ // This forces null terminated strings.
+ std::string LeftLine = Line1.str();
+ std::string RightLine = Line2.str();
+ EXPECT_STREQ(LeftLine.data(), RightLine.data());
----------------
Dumb question maybe, but why can't you do EXPECT_EQ(Line1, Line2)?
================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:58
+TEST(LLVMTAPI, YAMLReadsSymbols) {
+ const unsigned int NUM_SYMS = 5;
+ const char DATA[] = "--- !tapi-tbe-v1\n"
----------------
This should be a size_t, to match the return type of size(), although really, since it's only used once, I'm not sure there's any benefit having it out-of-line.
Also, I'm pretty sure the name should be camel-case, not all caps, ditto with DATA below.
Same comments apply to other tests.
================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:80
+
+ auto SymBar = Stub.Symbols[0];
+ EXPECT_STREQ(SymBar.StName.c_str(), "bar");
----------------
It would make more sense to me to check these symbols in the order they appear (i.e. Stub.Symbols[0], Stub.Symbols[1], etc. Is there a particular reason you're checking only 3, and not all 5? Alternatively, why are there 5 symbols at all?
================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:106
+
+TEST(LLVMTAPI, YAMLReadsArchitecture) {
+ const char DATA[] = "--- !tapi-tbe-v1\n"
----------------
You could reduce unnecessary duplication, and remove this and the next test by just testing SoName and Arch in YAMLReadsSymbols (and rename that test accordingly). I'm not sure having them separate gives you anything.
================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:133
+ // Can't read: wrong format/version.
+ const char DATA[] = "--- !ti-tbe-v42\n"
+ "SoName: test.so\n"
----------------
Maybe make it v0, to future-proof this test case.
================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:149
+ "Symbols: \n"
+ " bar: { Type: Object, Size: 42 }\n"
+ " baz: { Type: Object, Size: 3 }\n"
----------------
Similar comment to above, do you need 5 symbols? Could we have fewer?
Repository:
rL LLVM
https://reviews.llvm.org/D53051
More information about the llvm-commits
mailing list