[PATCH] D53051: [llvm-tapi] initial commit, supports ELF text stubs

[Armando Montanez via Phabricator via llvm-commits]

amontanez added a comment.

Thanks for being patient with me, I'm really appreciating your feedback!



================
Comment at: llvm/lib/TAPI/TBEHandler.cpp:74
+    if (Symbol.Type == ELFSymbolType::NoType) {
+      IO.mapOptional("Size", Symbol.Size, (uint64_t)0u);
+    } else if (Symbol.Type == ELFSymbolType::Func) {
----------------
jhenderson wrote:
> `(uint64_t)0u` looks weird to me. Do you need to be explicit about the type here?
Yes, otherwise it's ambiguous. I believe the the `u` can be safely dropped.


================
Comment at: llvm/lib/TAPI/TBEHandler.cpp:115
+    IO.mapRequired("SoName", Stub.SoName);
+    IO.mapRequired("Arch", (ELFArchMapper &)Stub.Arch);
+    IO.mapOptional("Symbols", Stub.Symbols);
----------------
jhenderson wrote:
> I'm guessing this is some magic in the YAML code? I don't really understand it, if I'm honest.
Yes. YAML can't tell the difference between `ELFArch` and `uint16_t` since `ELFArch` is just a typedef based on `uint16_t`. `uint16_t` has a default YAML implementation, so there must be a `LLVM_YAML_STRONG_TYPEDEF` to define a distinct type with overridden implementation. I avoided using the strong YAML typedef in ELFStub because things get rather messy if you try to use it for things other than YAML. Hence, the need for the reference cast here. It forces our standard type to be treated as a distinct YAML type.


================
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());
----------------
jhenderson wrote:
> amontanez wrote:
> > jhenderson wrote:
> > > Dumb question maybe, but why can't you do EXPECT_EQ(Line1, Line2)?
> > I tried that and it is functionally correct. Unfortunately, the output when a check fails is difficult to read.
> Okay, makes sense.
> 
> Aside: I reckon StringRef should be null terminated - it's a reference to a string, either a C-style literal one (complete with '\0') or a std::string (which is null-terminated in modern C++).
Normally it would be, but as I understand it's not guaranteed to be. Since it's just a reference to a character array, it's not null terminated if the reference is to a substring that isn't null terminated. Since I'm splitting on '\n', the last character of the substring isn't a null terminator. I updated the code comment to correct my wording.


================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:118
+TEST(LLVMTAPI, YAMLReadsNoSyms) {
+  const unsigned int NUM_SYMS = 0;
+  const char Data[] = "--- !tapi-tbe-v1\n"
----------------
jhenderson wrote:
> Move this inline.
Sorry, that slipped through.


Repository:
  rL LLVM

https://reviews.llvm.org/D53051