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

Wed Oct 31 11:44:45 PDT 2018

amontanez added a comment.

In https://reviews.llvm.org/D53051#1281689, @echristo wrote:

> Random drive-by bikeshed:
>
> TBEHandlerV1 is probably less than an ideal name for either an implementation or a filename. I'd suggest just calling it TBEHandler and making it possible to version and then subclass later if necessary.
>
> Thoughts?
>
> -eric

Good point, I got a bit ahead of myself there.

================
Comment at: llvm/include/llvm/TAPI/ELFStubFile.h:38
+  // Type information is a nibble, so 16 is safely out of range.
+  unknown =   16,
+};
----------------
jakehehrlich wrote:
> What is this for?
This deals with other symbol types like STT_SECTION, STT_FILE, STT_COMMON, STT_LOOS, STT_HIOS, STT_LOPROC, STT_HIPROC. Those are defaulted to unknown so as not to produce additional noise. Types that need to be present for ABI purposes should have explicit types. It's worth having a discussion about whether or not this is a good design choice.

================
Comment at: llvm/lib/TAPI/TBEHandlerV1.cpp:36
+
+template<> struct ScalarTraits<ELFArchMapper> {
+  static void output(const ELFArchMapper &value, void*,
----------------
jakehehrlich wrote:
> Can you use this? https://github.com/llvm-mirror/llvm/blob/master/lib/ObjectYAML/ELFYAML.cpp#L608
This particular key maps to ELF_EM: https://github.com/llvm-mirror/llvm/blob/cbcde934b39b8f45d4e796f5727dafb6cb6b4f1d/lib/ObjectYAML/ELFYAML.cpp#L58

The answer is yes we should be able to use that so long as we don't mind that everything will be formatted like the original enumeration members. (i.e. `EM_X86_64`, `EM_AARCH64`, etc).

================
Comment at: llvm/lib/TAPI/TBEHandlerV1.cpp:47
+
+  static StringRef input(StringRef scalar, void*, ELFArchMapper &value) {
+    if (scalar.equals("AARCH64"))
----------------
jakehehrlich wrote:
> jakehehrlich wrote:
> > You should probably implement this with a map defined as a static constant inside this function.
> Why does input return a StringRef? What is it used for? Please leave a comment explaining.
Per YAML IO documentation, you return an empty string on success and an error message on failure.

================
Comment at: llvm/lib/TAPI/TBEHandlerV1.cpp:93
+
+template <> struct MappingTraits<ELFStub> {
+  static void mapping(IO &io, ELFStub &stub) {
----------------
jakehehrlich wrote:
> How would multiple version be maintained here? Isn't there only one MappingTraits then?
For now, there's only one set of traits for anything read/writing with the ELF internal representation defined in ELFStubFile. There's two options I'm aware of for approaching versions in the future:

1. Wrap the internal representation components in classes that provide uniqueness and version independence.
2. Explicitly define a normalization/denormalization between the YAML input and the actual ELFStub.

Both require a common read/write interface that forks to the appropriate handler depending on the .tbe version.

================
Comment at: llvm/lib/TAPI/TBEHandlerV1.cpp:98-101
+    if (io.outputting() && stub.symbols.size() > 0)
+      io.mapRequired("SYMBOLS", stub.symbols);
+    else if (!io.outputting())
+      io.mapOptional("SYMBOLS", stub.symbols);
----------------
jakehehrlich wrote:
> Why can't you just always use mapOptional?
Map optional will always output 
```
Symbols:
...
```

when there are no symbols. This produces `error: not a mapping` when reading those results back in. I've resolved this by having curly braces as the value if the vector is empty.

================
Comment at: llvm/lib/TAPI/TBEHandlerV1.cpp:113
+  StringRef str = memBufferRef.getBuffer().trim();
+  // startswith() and endswith() aren't options as they break testing via lit.
+  if (str.find("--- !tapi-tbe-v1\n") != StringLiteral::npos
----------------
jakehehrlich wrote:
> This comment indicates this isn't the right way to do this. In general it is perfectly fine to attempt to read in a file and then handle the error without checking if it's going to work first.
Removed for now.

================
Comment at: llvm/lib/TAPI/TBEHandlerV1.cpp:136
+
+std::vector<std::string> TBEHandlerV1::getSupportedFileExtensions() const {
+  return {".tbe"};
----------------
jakehehrlich wrote:
> I'm not sure if they have this in their patch or how it's used exactly, but it should only be used as a hint (perhaps to choose which format parsing should attempt first) and should never be required. I'm inclined to throw it out all together especially since binary formats should quickly error out if the MemoryBuffer doesn't contain the correct magic at the start.
As it is in llvm-tapi, the only purpose this serves is to call getSupportedFileExtensions() from the Registry (not in this patch) that then calls the same function of all the readers/writers to produce a list of all the supported extensions. It's a way to have extension hints that can be clearly associated with a given reader/writer, and is never used to determine file format. I'll omit this function for now as it isn't really relevant to this patch.

================
Comment at: llvm/lib/TAPI/TBEHandlerV1.cpp:152
+
+Error TBEHandlerV1::writeFile(raw_ostream &os, const File *file) {
+  auto *inFile = dyn_cast_or_null<ELFStubFile>(file);
----------------
jakehehrlich wrote:
> Looks like there's no good way to avoid using a raw_ostream but I'll leave you with this bit of advice to make sure the problem doesn't leak to the rest of the interfaces. When writing ELF files you'll want to use something like a Filebuffer and not an raw_ostream so whatever abstraction you land on for writing out the different formats needs to be generic to those two options. I'm not aware of a good abstraction to account for this at the moment but I'll think about it.
My first thought is to have everything write to a FileOutputBuffer as their common interfaces, and just have YAML writers write to it through an adapter. I'm not sure if this is the right approach though, and would definitely be suboptimal for YAML writing.

Repository:
  rL LLVM

https://reviews.llvm.org/D53051