[PATCH] D60974: Clang IFSO driver action.

Tue May 7 08:54:25 PDT 2019

compnerd added inline comments.

================
Comment at: clang/include/clang/Driver/Options.td:626
   HelpText<"Use the LLVM representation for assembler and object files">;
+def emit_ifso : Flag<["-"], "emit-interface-stubs">, Flags<[CC1Option]>, Group<Action_Group>,
+  HelpText<"Generate Inteface Stub Files.">;
----------------
I thought that we were going to add `experimental` to this for the time being?

================
Comment at: clang/include/clang/Driver/Options.td:628
+  HelpText<"Generate Inteface Stub Files.">;
+def ifso_version_EQ : Joined<["-"], "interface-stubs-version=">, Flags<[CC1Option]>;
 def exported__symbols__list : Separate<["-"], "exported_symbols_list">;
----------------
Personally, I have a slight preference for the separate version rather than the joined.

================
Comment at: clang/include/clang/Driver/Types.def:91
 TYPE("ast",                      AST,          INVALID,         "ast",   "u")
+TYPE("ifs",                      IFS,          INVALID,         "ifs",   "u")
 TYPE("pcm",                      ModuleFile,   INVALID,         "pcm",   "u")
----------------
What about `ifo` instead of `ifs`?

================
Comment at: clang/include/clang/Frontend/FrontendActions.h:128
+};
+// Support different ifso formats this way:
+class GenerateIfsoObjYamlExpV1Action : public GenerateIFSOAction {
----------------
Not sure if the comment adds anything.  But a newline before these would be nice.

================
Comment at: clang/include/clang/Frontend/FrontendActions.h:134
+};
+class GenerateIfsoTbeExpV1Action : public GenerateIFSOAction {
+protected:
----------------
A newline between the classes would be nice.

================
Comment at: clang/include/clang/Frontend/FrontendOptions.h:91
+  /// Generate Interface Stub Files.
+  GenerateIfsoObjYamlExpV1,
+  GenerateIfsoTbeExpV1,
----------------
As per LLVM style, initialisms are not downcased.  This should be `GenerateIfSoObjYAMLExpV1`.  I think I prefer `GenerateInterfaceYAMLExpV1`

================
Comment at: clang/include/clang/Frontend/FrontendOptions.h:92
+  GenerateIfsoObjYamlExpV1,
+  GenerateIfsoTbeExpV1,
+
----------------
Similar.  I think I prefer `GenerateInterfaceTBEExpV1`

================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3590
+            Twine("-interface-stubs-version=") +
+            Args.getLastArgValue(options::OPT_ifso_version_EQ)));
+      }
----------------
Please ensure that the value being passed is valid or emit a diagnostic.

================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1649
+    case OPT_emit_ifso:
+      Opts.ProgramAction = frontend::GenerateIfsoTbeExpV1;
+      if (Args.hasArg(OPT_ifso_version_EQ))
----------------
Can we make this required to be specified instead of defaulting please?  I think that it is more clear.

================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1651
+      if (Args.hasArg(OPT_ifso_version_EQ))
+        if(Args.getLastArgValue(OPT_ifso_version_EQ) == "experimental-ifo-elf-v1")
+          Opts.ProgramAction = frontend::GenerateIfsoObjYamlExpV1;
----------------
Please use a covered `StringSwitch` here.  Since this is in the frontend,  I think that we should have a test that the driver diagnoses invalid values.

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:164

+class IFSOFunctionsConsumer : public ASTConsumer {
+  CompilerInstance &Instance;
----------------
This does not belong here.  Please extract it into a separate file, much like the serialization is extracted into a separate file.

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:179
+  };
+  using MangledSymbols = std::map<const NamedDecl *, MangledSymbol>;
+
----------------
Hmm, I wonder if `DenseSet` is more appropriate here.  Is there some reason that I am overlooking for supporting duplicate entries?  In fact, since the key is a pointer, you could even do a `DenseMap`.

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:182
+  template <typename T>
+  bool WriteNamedDecl(const NamedDecl *ND, MangledSymbols &Symbols, int RDO) {
+    if (!isa<T>(ND))
----------------
I'm not sure I like this very much.  You are templating over a type a function that needs to walk the hierarchy for `NamedDecl`.  This is going to re-emit the function and without LTO cause a large `.text` contribution for something that should really just be a jump table.  Would it not be possible to use a covered switch on the `kindof()` in the `NamedDecl`.  The covering should ensure that future changes would allow us to find this site.

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:189
+      return true;
+    if (ND->getVisibility() != DefaultVisibility)
+      return true;
----------------
I understand this, but, it may be nice to have a comment explaining why we only consider default visibility (hidden visibility interfaces cannot be seen by consumers, and protected mode visibility in ELF does not participate in linking, only loading).

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:197
+        return true;
+    index::CodegenNameGenerator CGNameGen(ND->getASTContext());
+    std::string MangledName = CGNameGen.getName(ND);
----------------
Newline before this would be nice.

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:199
+    std::string MangledName = CGNameGen.getName(ND);
+    uint8_t Type = llvm::ELF::STT_FUNC;
+    uint8_t Binding = llvm::ELF::STB_GLOBAL;
----------------
This is confusing, can you rewrite this as, dropping L204-205:

```
uint8_t Type = isa<VarDecl>(ND) ? llvm::ELF::STT_OBJECT : llvm::ELF::STT_FUNC;
```

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:203
+        ND->isWeakImported())
+      Binding = llvm::ELF::STB_WEAK;
+    if (isa<VarDecl>(ND))
----------------
Again, I think that this is slightly confusing, and would prefer a ternary.

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:207
+
+    Symbols.insert(std::pair<const NamedDecl *, MangledSymbol>(
+        ND, MangledSymbol(MangledName, Type, Binding)));
----------------
Can you not use a universal initialiser?  In the case you cannot, `std::make_pair` would be nicer to use for the type deduction.

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:214
+      ND->dump();
+    }
+    return true;
----------------
debugging left over?

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:219
+  template <typename T>
+  bool HandleSomeDecl(const NamedDecl *ND, MangledSymbols &Symbols, int RDO) {
+    if (!isa<T>(ND))
----------------
Similar to the previous case, I am slightly worried about the size contributions here.  Why not a covered switch?

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:222
+      return false;
+    for (auto *I : cast<T>(ND)->decls())
+      HandleNamedDecl(dyn_cast<NamedDecl>(I), Symbols, RDO);
----------------
`const auto *D`?

This also makes very little sense to the casual reader.  This cast is very confusing.  There is no reason to assume that an arbitrary `NamedDecl` is a `DeclContext`.  It is only made even more confusing by the fact that the decl that is retrieved is a pointer - since that is an implementation detail of the `decl_iterator` in the `decl_range` returned by `decls` which is implicitly dereferenced by the range based for loop.  Making the type explicit by the means of the covered switch and an explicit cast to the `DeclContext` should help in clearing that up.

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:223
+    for (auto *I : cast<T>(ND)->decls())
+      HandleNamedDecl(dyn_cast<NamedDecl>(I), Symbols, RDO);
+    return true;
----------------
Hmm, do we have a guarantee that the decl is named?  Could you not hit a `_Static_assert` or `static_assert` in this traversal?  Or if it is a C++ context, an `extern "C"` block?  What about `#pragma comment(...)`?  Please add test cases for these.

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:227
+
+  template <typename T>
+  bool HandleSomeDeclSpec(const NamedDecl *ND, MangledSymbols &Symbols,
----------------
Again, the templating here worries me.

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:232
+      return false;
+    for (auto *I : cast<T>(ND)->specializations())
+      HandleNamedDecl(dyn_cast<NamedDecl>(I), Symbols, RDO);
----------------
Hmm, what guarantee do we have that the templated type is a template type?

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:254
+      return true;
+    // While IFSOs are in the development stage, it's probably best to catch
+    // anything that's not a VarDecl or Template/FunctionDecl.
----------------
Please put this in an `LLVM_DEBUG`.

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:269
+      bool VisitNamedDecl(NamedDecl *ND) {
+        if (auto *FD = dyn_cast<FunctionDecl>(ND))
+          (FD->isLateTemplateParsed() ? LateParsedDecls : NamedDecls)
----------------
const?  I think that this might be easier to parse for the reader as:

```
if (const auto *FD = dyn_cast<FunctionDecl>(ND))
  if (FD->isLateTemplateParsed())
    return LateParsedDecls.insert(FD).first != LateParsedDecls.end();
  else
    return NamedDecls.insert(FD).first != NamedDecls.end();

if (const auto *VD = dyn_cast<ValueDecl>(ND))
  return ValueDecls.insert(VD).first != ValueDecls.end();

return NamedDecls.insert(ND) != NamedDecls.end();
```

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:292
+    if (Instance.getLangOpts().DelayedTemplateParsing) {
+      clang::Sema &sema = Instance.getSema();
+      for (const auto *FD : v.LateParsedDecls) {
----------------
`S` is the traditional initialism.

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:297
+        sema.LateTemplateParser(sema.OpaqueParser, LPT);
+        HandleNamedDecl(FD, Symbols, (FromTU | IsLate));
+      }
----------------
Typo of `||`?  The field is boolean not a mask.

================
Comment at: clang/lib/Frontend/FrontendActions.cpp:341
+         << "\nSymbols:\n";
+      for (auto E : Symbols) {
+        const MangledSymbol &Symbol = E.second;
----------------
`const auto &E`?  I wish that we had structured decomposition already.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60974