[libc] Use entrypoints.txt as the single source of list of functions for a platform.

[michaelrj]

Summary

The function listings in api.td are removed. The same lists are now deduced using the information
in entrypoints.txt.

[sivachandra]

libc/cmake/modules/LLVMLibCHeaderRules.cmake

94

Align with the other lines in the command?

libc/spec/llvm_libc_ext.td

1 ↗

(On Diff #297687)

Can you move the changes in this file to a separate patch?

libc/spec/spec.td

43 ↗

(On Diff #297687)

Changes in this file and stdc.td can also go into a separate patch.

56 ↗

(On Diff #297687)

Remove this comment?

libc/spec/stdc.td

4 ↗

(On Diff #297687)

I did not get this comment. Can you explain a bit more?

libc/utils/HdrGen/EntrypointsLoader.cpp

29 ↗

(On Diff #297687)

The error message should include the path to the file.

31 ↗

(On Diff #297687)

Why is a new required here? Also, ins't it a memory leak? Isn't a simple stack variable sufficient?

62 ↗

(On Diff #297687)

We should not parse the header name here. Instead, we can use the spec to map the functions to the header file in the caller.

libc/utils/HdrGen/EntrypointsLoader.h

1 ↗

(On Diff #297687)

Call this file (and the .cpp file) EntrypointsReader.[h|cpp]?

19 ↗

(On Diff #297687)

Seems like this class is not of much use. Remove it?

26 ↗

(On Diff #297687)

Call this function getFunctionsFromEntrypointsFile? Also, we don't need to pass the HeaderName. Just get all the functions and let the caller pick the function corresponding to the header file.

[michaelrj]

Change the source of header/function mappings

Now the mappings describing what function goes to what header is
coming from the spec and not from entrypoints.txt.
Also included are some formatting fixes.

[lntue]

libc/spec/llvm_libc_ext.td

29 ↗

(On Diff #297941)

alignment

[sivachandra]

Overall LGTM but I have couple more comments over the previous ones.

libc/utils/HdrGen/Main.cpp

25

s/your system/your target

libc/utils/HdrGen/PublicAPICommand.cpp

88–96

When running the PublicAPICommand, the FunctionSpecMap will be for the header file we are generating. So, it should still be an error if a function is not found, no?

[michaelrj]

Many small changes

Moved the changes in libc/spec into their own commit.
Renamed EntrypointsLoader -> EntrypointsReader
Various formatting fixes.

[michaelrj]

libc/spec/llvm_libc_ext.td

29 ↗

(On Diff #297941)

since I'm moving the changes in this file to a different commit the fix will be there, but it has been fixed.

libc/spec/stdc.td

4 ↗

(On Diff #297687)

since I'm moving the changes in this file to a different commit I'll update the comment there.

libc/utils/HdrGen/EntrypointsLoader.cpp

31 ↗

(On Diff #297687)

Oops, you're right that shouldn't be on the heap. I was for some reason thinking that the set was going to be passed by reference and not by value.

libc/utils/HdrGen/PublicAPICommand.cpp

88–96

The FunctionSpecMap is indeed for the header file we're generating, but that line is checking if each function in the list Functions is in FunctionSpecMap. Previously the list in Functions (or G.Functions) was supposed to be identical to that list, but since we're now ignoring what header file we're looking for in getFunctionsFromEntrypointsFile, the list Functions contains every function listed in entrypoints.txt. When I run the code with this as an error it crashes when trying to load a header file because not all functions listed are in that header file.

[michaelrj]

addressed comments.

[sivachandra]

Still LGTM. Few more nitty inline comments.

libc/utils/HdrGen/EntrypointsReader.cpp

28 ↗

(On Diff #297981)

If a statement ends up spanning multiple lines, then use braces for the enclosing if/for etc.

41 ↗

(On Diff #297981)

Trim after dropping the comment? That is, after line #45?

43 ↗

(On Diff #297981)

No braces here.

47 ↗

(On Diff #297981)

Ditto.

51 ↗

(On Diff #297981)

This part of the code makes me nervous. Nothing wrong with you've got, but just that we are parsing a CMake file. We already "parse" the entrypoint list here: https://github.com/llvm/llvm-project/blob/master/libc/CMakeLists.txt#L76. So, technically we can just pass that list as a command line argument to hdrgen. I am not sure how portable that will be because of the command line length. Lets keep this for now. We can switch to command line args later if required.

libc/utils/HdrGen/PublicAPICommand.cpp

88–96

Ah, got it. Thanks for explaining. Instead of a dangling comment, can you format like this:

if (...) {
  // comment
  // comment
  // ...
  continue;
}

Also, try to use correct grammar like beginning sentences with Upper case words. You don't need to explain previous code (in fact, it will be confusing if you do) but explain the current code.

[sivachandra]

libc/utils/HdrGen/EntrypointsReader.cpp

51 ↗

(On Diff #297981)

I did some more digging and this is what I found: https://github.com/llvm/llvm-project/blob/master/libc/test/src/CMakeLists.txt#L31
Which is an existing example of passing the list of functions as command line arguments. So, we should probably do that and not use a custom parser like this.

[michaelrj]

Remove the redundant new parser

EntrypointsReader was deleted because the cmake phase was
already doing that parsing. Now the list of entrypoints is
being passed as arguments to the headergen, just like was
being done for the integration tests in
libc/utils/HdrGen/PrototypeTestGen.

[sivachandra]

libc/utils/HdrGen/Command.h

49 ↗

(On Diff #298240)

Instead of passing the entrypoint list to all commands, can perhaps pass it to the PublicAPICommand constructor here: https://github.com/llvm/llvm-project/blob/master/libc/utils/HdrGen/Generator.cpp#L43

libc/utils/HdrGen/Main.cpp

28

desc should be proper English description. value_desc can be what you have listed as <list of entrypoints>.

Also, to be consistent with this https://github.com/llvm/llvm-project/blob/master/libc/utils/HdrGen/PrototypeTestGen/PrototypeTestGen.cpp, use just e for the option name.

libc/utils/HdrGen/PublicAPICommand.cpp

88–96

I think you have not addressed this?

[sivachandra]

libc/spec/llvm_libc_ext.td

16 ↗

(On Diff #298240)

Sorry for not pointing this out earlier. Remove this edit from this change.

[michaelrj]

Fix formatting and move data path to PublicAPICommand

Now PublicAPICommand has a constructor that takes the list of entrypoints,
as opposed to it getting that data as an argument in the run function.
This means we don't need an extraneous argument for IncludeFileCommand.

Also renamed --ent to --e for consistency.

libc/utils/HdrGen/PublicAPICommand.cpp

88–96

Addressed in the new version.

[sivachandra]

OK after addressing the last set of nits.

libc/spec/stdc.td

2 ↗

(On Diff #298265)

Remove this edit from this patch.

libc/utils/HdrGen/Generator.h

34

Make this a const reference:

const std::vector<std::string> &EntrypointNameList;

49

A const reference argument here as well. Also, instead of EntrypointNamesOption, EN is OK.

libc/utils/HdrGen/PublicAPICommand.h

30

A const reference member here as well?

35

A const reference argument.

[sivachandra]

Sorry, one more thing. Please update the commit message according the latest version of this patch.

[michaelrj]

Fixed nits.

libc/CMakeLists.txt

message(WARNING "
     This is not recommended, to enable set LLVM_LIBC_ENABLE_LINTING to ON
     (pass -DLLVM_LIBC_ENABLE_LINTING=ON to cmake).")
 endif()
 
 include(CMakeParseArguments)
 include(LLVMLibCRules)
 include(LLVMLibCCheckCpuFeatures)
 
-add_subdirectory(include)
-add_subdirectory(config)
-
 include("${LIBC_SOURCE_DIR}/config/${LIBC_TARGET_OS}/${LIBC_TARGET_MACHINE}/entrypoints.txt")
 include("${LIBC_SOURCE_DIR}/config/${LIBC_TARGET_OS}/${LIBC_TARGET_MACHINE}/headers.txt")
 
 set(TARGET_ENTRYPOINT_NAME_LIST "")
 foreach(entrypoint IN LISTS TARGET_LIBC_ENTRYPOINTS TARGET_LIBM_ENTRYPOINTS)
   string(FIND ${entrypoint} "." last_dot_loc REVERSE)
   if(${last_dot_loc} EQUAL -1)
     message(FATAL "Invalid entrypoint target name ${entrypoint}; Expected a '.' "
                   "(dot) in the name.")
   endif()
   math(EXPR name_loc "${last_dot_loc} + 1")
   string(SUBSTRING ${entrypoint} ${name_loc} -1 entrypoint_name)
   list(APPEND TARGET_ENTRYPOINT_NAME_LIST ${entrypoint_name})
 endforeach()
 
+add_subdirectory(include)
+add_subdirectory(config)
 add_subdirectory(src)
 add_subdirectory(utils)
 
 # The loader can potentially depend on the library components so add it
 # after the library implementation directories.
 add_subdirectory(loader)
 
 # The lib and test directories are added at the very end as tests

libc/cmake/modules/LLVMLibCHeaderRules.cmake

function(add_gen_header target_name)
   if(ADD_GEN_HDR_PARAMS)
     list(APPEND replacement_params "--args" ${ADD_GEN_HDR_PARAMS})
   endif()
 
   set(gen_hdr_script "${LIBC_BUILD_SCRIPTS_DIR}/gen_hdr.py")
 
   file(GLOB td_includes ${LIBC_SOURCE_DIR}/spec/*.td)
 
+  set(ENTRYPOINT_NAME_LIST_ARG ${TARGET_ENTRYPOINT_NAME_LIST})
+  list(TRANSFORM ENTRYPOINT_NAME_LIST_ARG PREPEND "--e=")
+
   add_custom_command(
     OUTPUT ${out_file}
     COMMAND $<TARGET_FILE:libc-hdrgen> -o ${out_file} --header ${ADD_GEN_HDR_GEN_HDR}
             --def ${in_file} ${replacement_params} -I ${LIBC_SOURCE_DIR}
-            ${LIBC_SOURCE_DIR}/config/${LIBC_TARGET_OS}/api.td
+           ${ENTRYPOINT_NAME_LIST_ARG}
 
     WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
     DEPENDS ${in_file} ${fq_data_files} ${td_includes}
             ${LIBC_SOURCE_DIR}/config/${LIBC_TARGET_OS}/api.td libc-hdrgen
   )
 
   get_fq_target_name(${target_name} fq_target_name)
   if(ADD_GEN_HDR_DEPENDS)
     get_fq_deps_list(fq_deps_list ${ADD_GEN_HDR_DEPENDS})
   endif()
   add_custom_target(
     ${fq_target_name}
     DEPENDS ${out_file} ${fq_deps_list}
   )
 endfunction(add_gen_header)

libc/config/linux/api.td

 def AssertAPI : PublicAPI<"assert.h"> {
   let Macros = [
     AssertMacro,
     StaticAssertMacro,
   ];
 }
 
 def CTypeAPI : PublicAPI<"ctype.h"> {
-  let Functions = [
-    "isalnum",
-    "isalpha",
-    "isblank",
-    "iscntrl",
-    "isdigit",
-    "isgraph",
-    "islower",
-    "isprint",
-    "ispunct",
-    "isspace",
-    "isupper",
-    "isxdigit",
-    "tolower",
-    "toupper",
-  ];
 }
 
 def MathErrHandlingMacro : MacroDef<"math_errhandling"> {
   let Defn = [{
     #ifndef math_errhandling
     #ifdef __FAST_MATH__
     #define math_errhandling 0
     #elif defined __NO_MATH_ERRNO__
let Macros = [
     IsFiniteMacro,
     IsInfMacro,
     IsNanMacro,
   ];
   let TypeDeclarations = [
     DoubleT,
     FloatT,
   ];
-  let Functions = [
-   "copysign",
-   "copysignf",
-   "copysignl",
-   "ceil",
-   "ceilf",
-   "ceill",
-   "cosf",
-   "fabs",
-   "fabsf",
-   "fabsl",
-   "floor",
-   "floorf",
-   "floorl",
-   "fmax",
-   "fmaxf",
-   "fmaxl",
-   "fmin",
-   "fminf",
-   "fminl",
-   "frexp",
-   "frexpf",
-   "frexpl",
-   "hypotf",
-   "logb",
-   "logbf",
-   "logbl",
-   "modf",
-   "modff",
-   "modfl",
-   "expf",
-   "exp2f",
-   "remainderf",
-   "remainder",
-   "remainderl",
-   "remquof",
-   "remquo",
-   "remquol",
-   "round",
-   "roundf",
-   "roundl",
-   "sincosf",
-   "sinf",
-   "sqrt",
-   "sqrtf",
-   "sqrtl",
-   "trunc",
-   "truncf",
-   "truncl",
-  ];
 }
 
 def StringAPI : PublicAPI<"string.h"> {
-  let Functions = [
-    "bzero",
-    "memchr",
-    "memcmp",
-    "memcpy",
-    "memmove",
-    "memrchr",
-    "memset",
-    "strcat",
-    "strchr",
-    "strcmp",
-    "strcoll",
-    "strcpy",
-    "strcspn",  
-    "strerror",
-    "strlen",
-    "strncat",
-    "strncmp",
-    "strncpy",
-    "strnlen",
-    "strpbrk",
-    "strrchr",
-    "strspn",
-    "strstr",
-    "strtok",
-    "strtok_r",
-    "strxfrm",   
-  ];
-
   let TypeDeclarations = [
     SizeT,
   ];
 
   let Macros = [
     NullMacro,
   ];
 }
 
 def StdIOAPI : PublicAPI<"stdio.h"> {
   let TypeDeclarations = [
     SizeT,
     FILE,
   ];
-
-  let Functions = [
-    "fwrite",
-  ];
 }
 
 def StdlibAPI : PublicAPI<"stdlib.h"> {
-  let Functions = [
-    "_Exit",
-    "abort",
-  ];
 }
 
 def ErrnoAPI : PublicAPI<"errno.h"> {
   let Macros = [
     ErrnoMacro,
     // We largely depend on linux/errno.h to give us the
     // various error macro definitions. However, some libc
     // implementations have chosen to provide definitions
let Macros = [
 
     // TODO: Add other MAP_* macros used by Linux.
   ];
 
   let TypeDeclarations = [
     SizeT,
     OffT,
   ];
-
-  let Functions = [
-    "mmap",
-    "munmap",
-  ];
 }
 
 def StructSigactionDefn : TypeDecl<"struct sigaction"> {
   let Decl = [{
     struct __sigaction {
       union {
         void (*sa_handler)(int);
         void (*sa_action)(int, siginfo_t *, void *);
def SighandlerTDefn : TypeDecl<"__sighandler_t"> {
   }];
 }
 
 def SignalAPI : PublicAPI<"signal.h"> {
   let TypeDeclarations = [
     StructSigactionDefn,
     SighandlerTDefn,
   ];
-
-  let Functions = [
-    "raise",
-    "sigaction",
-    "sigdelset",
-    "sigprocmask",
-    "sigemptyset",
-    "sigaddset",
-    "sigfillset",
-    "signal",
-  ];
 }
 
 def OnceFlag : TypeDecl<"once_flag"> {
   let Decl = [{
     typedef unsigned int once_flag;
   }];
 }
 
let Enumerations = [
     "mtx_recursive",
     "mtx_timed",
     "thrd_timedout",
     "thrd_success",
     "thrd_busy",
     "thrd_error",
     "thrd_nomem",
   ];
-
-  let Functions = [
-    "call_once",
-    "mtx_init",
-    "mtx_lock",
-    "mtx_unlock",
-    "thrd_create",
-    "thrd_join",
-  ];
 }
 
 def UniStdAPI : PublicAPI<"unistd.h"> {
   let TypeDeclarations = [
     SSizeT,
     SizeT,
   ];
-
-  let Functions = [
-    "write",
-  ];
 }

libc/utils/HdrGen/Generator.h

 } // namespace llvm
 
 namespace llvm_libc {
 
 class Command;
 
 class Generator {
   llvm::StringRef HeaderDefFile;
+  const std::vector<std::string> &EntrypointNameList;
   llvm::StringRef StdHeader;
   std::unordered_map<std::string, std::string> &ArgMap;
 
   std::unique_ptr<Command> IncludeFileCmd;
   std::unique_ptr<Command> PublicAPICmd;
 
   Command *getCommandHandler(llvm::StringRef CommandName);
 
   void parseCommandArgs(llvm::StringRef ArgStr, ArgVector &Args);
 
   void printError(llvm::StringRef Msg);
 
 public:
-  Generator(const std::string &DefFile, const std::string &Header,
+  Generator(const std::string &DefFile, const std::vector<std::string> &EN,
+            const std::string &Header,
             std::unordered_map<std::string, std::string> &Map)
-      : HeaderDefFile(DefFile), StdHeader(Header), ArgMap(Map) {}
+      : HeaderDefFile(DefFile), EntrypointNameList(EN), StdHeader(Header),
+        ArgMap(Map) {}
 
   void generate(llvm::raw_ostream &OS, llvm::RecordKeeper &Records);
 };
 
 } // namespace llvm_libc
 
 #endif // LLVM_LIBC_UTILS_HDRGEN_GENERATOR_H

libc/utils/HdrGen/Generator.cpp

 
 Command *Generator::getCommandHandler(llvm::StringRef CommandName) {
   if (CommandName == IncludeFileCommand::Name) {
     if (!IncludeFileCmd)
       IncludeFileCmd = std::make_unique<IncludeFileCommand>();
     return IncludeFileCmd.get();
   } else if (CommandName == PublicAPICommand::Name) {
     if (!PublicAPICmd)
-      PublicAPICmd = std::make_unique<PublicAPICommand>();
+      PublicAPICmd = std::make_unique<PublicAPICommand>(EntrypointNameList);
     return PublicAPICmd.get();
   } else {
     return nullptr;
   }
 }
 
 void Generator::parseCommandArgs(llvm::StringRef ArgStr, ArgVector &Args) {
   if (!ArgStr.contains(',') && ArgStr.trim(' ').trim('\t').size() == 0) {

libc/utils/HdrGen/Main.cpp

 
 namespace {
 
 llvm::cl::opt<std::string>
     HeaderDefFile("def", llvm::cl::desc("Path to the .h.def file."),
                   llvm::cl::value_desc("<filename>"), llvm::cl::Required);
 llvm::cl::opt<std::string> StandardHeader(
     "header",
     llvm::cl::desc("The standard header file which is to be generated."),
     llvm::cl::value_desc("<header file>"));
+llvm::cl::list<std::string> EntrypointNamesOption(
+    "e", llvm::cl::value_desc("<list of entrypoints>"),
+    llvm::cl::desc(
+        "Each --e is one entrypoint (generated from entrypoints.txt)"),
+    llvm::cl::OneOrMore);
 llvm::cl::list<std::string> ReplacementValues(
     "args", llvm::cl::desc("Command separated <argument name>=<value> pairs."),
     llvm::cl::value_desc("<name=value>[,name=value]"));
 
 void ParseArgValuePairs(std::unordered_map<std::string, std::string> &Map) {
   for (std::string &R : ReplacementValues) {
     auto Pair = llvm::StringRef(R).split('=');
     Map[std::string(Pair.first)] = std::string(Pair.second);
   }
 }
 
 } // anonymous namespace
 
 namespace llvm_libc {
 
 bool HeaderGeneratorMain(llvm::raw_ostream &OS, llvm::RecordKeeper &Records) {
   std::unordered_map<std::string, std::string> ArgMap;
   ParseArgValuePairs(ArgMap);
-  Generator G(HeaderDefFile, StandardHeader, ArgMap);
+  Generator G(HeaderDefFile, EntrypointNamesOption, StandardHeader, ArgMap);
   G.generate(OS, Records);
 
   return false;
 }
 
 } // namespace llvm_libc
 
 int main(int argc, char *argv[]) {
   llvm::cl::ParseCommandLineOptions(argc, argv);
   return TableGenMain(argv[0], &llvm_libc::HeaderGeneratorMain);
 }

libc/utils/HdrGen/PublicAPICommand.h

 class Record;
 class RecordKeeper;
 
 } // namespace llvm
 
 namespace llvm_libc {
 
 class PublicAPICommand : public Command {
+private:
+  const std::vector<std::string> &EntrypointNameList;
+
 public:
   static const char Name[];
 
+  PublicAPICommand(const std::vector<std::string> &EntrypointNames)
+      : EntrypointNameList(EntrypointNames) {}
+
   void run(llvm::raw_ostream &OS, const ArgVector &Args,
            llvm::StringRef StdHeader, llvm::RecordKeeper &Records,
            const Command::ErrorReporter &Reporter) const override;
 };
 
 } // namespace llvm_libc
 
 #endif // LLVM_LIBC_UTILS_HDRGEN_PUBLICAPICOMMAND_H

libc/utils/HdrGen/PublicAPICommand.cpp

static void dedentAndWrite(llvm::StringRef Text, llvm::raw_ostream &OS) {
   for (llvm::StringRef L : Lines) {
     if (L.size() >= shortest_indent)
       OS << L.drop_front(shortest_indent) << '\n';
   }
 }
 
 namespace llvm_libc {
 
-void writeAPIFromIndex(APIIndexer &G, llvm::raw_ostream &OS) {
+void writeAPIFromIndex(APIIndexer &G,
+                       std::vector<std::string> EntrypointNameList,
+                       llvm::raw_ostream &OS) {
   for (auto &Pair : G.MacroDefsMap) {
     const std::string &Name = Pair.first;
     if (G.MacroSpecMap.find(Name) == G.MacroSpecMap.end())
       llvm::PrintFatalError(Name + " not found in any standard spec.\n");
 
     llvm::Record *MacroDef = Pair.second;
     dedentAndWrite(MacroDef->getValueAsString("Defn"), OS);
 
for (const auto &Name : G.Enumerations) {
     } else {
       OS << " = " << Value << ",\n";
     }
   }
   if (G.Enumerations.size() != 0)
     OS << "};\n\n";
 
   OS << "__BEGIN_C_DECLS\n\n";
-  for (auto &Name : G.Functions) {
-    if (G.FunctionSpecMap.find(Name) == G.FunctionSpecMap.end())
-      llvm::PrintFatalError(Name + " not found in any standard spec.\n");
+  for (auto &Name : EntrypointNameList) {
+    if (G.FunctionSpecMap.find(Name) == G.FunctionSpecMap.end()) {
+      continue; // Functions that aren't in this header file are skipped as
+                // opposed to erroring out because the list of functions being
+                // iterated over is the complete list of functions with
+                // entrypoints. Thus this is filtering out the functions that
+                // don't go to this header file, whereas the other, similar
+                // conditionals above are more of a sanity check.
+    }
 
     llvm::Record *FunctionSpec = G.FunctionSpecMap[Name];
     llvm::Record *RetValSpec = FunctionSpec->getValueAsDef("Return");
     llvm::Record *ReturnType = RetValSpec->getValueAsDef("ReturnType");
 
     OS << G.getTypeAsString(ReturnType) << " " << Name << "(";
 
     auto ArgsList = FunctionSpec->getValueAsListOfDefs("Args");
void PublicAPICommand::run(llvm::raw_ostream &OS, const ArgVector &Args,
                            llvm::StringRef StdHeader,
                            llvm::RecordKeeper &Records,
                            const Command::ErrorReporter &Reporter) const {
   if (Args.size() != 0) {
     Reporter.printFatalError("public_api command does not take any arguments.");
   }
 
   APIIndexer G(StdHeader, Records);
-  writeAPIFromIndex(G, OS);
+  writeAPIFromIndex(G, EntrypointNameList, OS);
 }
 
 } // namespace llvm_libc