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

[Armando Montanez via Phabricator via llvm-commits]

amontanez added a comment.

Hopefully this addresses most of the comments.



================
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,
----------------
jhenderson wrote:
> Maybe drop the V1/version 1.0 here?
This is something we wouldn't want to change after it lands, so if there's a desire for it to be TBE_V1 when a TBE_V2 file type appears it might be best to keep it as TBE_V1. This is also the only means to distinguish or select version after a file has been read into an ELFStub, so grouping all versions under the same could cause issues. Since this enum will likely be merged with Apple's Mach-O TAPI version, it mimics the naming 
`TBD_V1`, `TBD_V2`, etc. for consistency.


================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:73
+  }
+  explicit ELFStubFile(FileType Type, ELFStub &SourceStub) {
+    this->Type = Type;
----------------
jhenderson wrote:
> This should probably go into a .cpp, given its relative complexity.
I've moved the contents of this constructor into ELFStub's copy constructor for now. I have a suspicion that ELFStub will become more class-like as llvm-tapi evolves, so I've factored it out into another file and made it a class.


================
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"
----------------
jhenderson wrote:
> Most (all?) of these includes can be replaced with forward-declarations.
It appears that ELFStubFile is the only one that can't be replaced since FileType is an enum.


================
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"},
----------------
jhenderson wrote:
> 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.
Looks like a DenseMap is a reasonable choice.


================
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));
----------------
jhenderson wrote:
> You can use createStringError here too. I'm wondering whether it should just be an assertion though, not sure about that.
Considering it wouldn't make sense to recover from that error, I think it's reasonable to change it to an assert. If this was writing _to_ a file (rather than _from_ a file object to a raw_ostream), creating an error would make more sense since it could be more than a programmatic error.


================
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:
> 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.


================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:80
+
+  auto SymBar = Stub.Symbols[0];
+  EXPECT_STREQ(SymBar.StName.c_str(), "bar");
----------------
jhenderson wrote:
> 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?
Having 3 or more symbols checks that everything was correctly sorted. Five was overkill for that purpose alone, but I've updated the test to cover more code by testing NoType and TLS too (making for four symbol types tested against, with NoType tested twice to ensure size being optional produces expected output. They're now checked in order.


================
Comment at: llvm/unittests/TAPI/YAMLTest.cpp:149
+      "Symbols:         \n"
+      "  bar:             { Type: Object, Size: 42 }\n"
+      "  baz:             { Type: Object, Size: 3 }\n"
----------------
jhenderson wrote:
> Similar comment to above, do you need 5 symbols? Could we have fewer?
Reduced to three to ensure sorted while checking a few other things.


Repository:
  rL LLVM

https://reviews.llvm.org/D53051