Page MenuHomePhabricator

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

Authored by michaelrj on Oct 12 2020, 1:45 PM.

Details

Summary

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

Diff Detail

Event Timeline

michaelrj created this revision.Oct 12 2020, 1:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2020, 1:45 PM
michaelrj requested review of this revision.Oct 12 2020, 1:45 PM
sivachandra added inline comments.Oct 13 2020, 1:12 PM
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 updated this revision to Diff 297941.Oct 13 2020, 1:17 PM

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 added a subscriber: lntue.Oct 13 2020, 1:27 PM
lntue added inline comments.
libc/spec/llvm_libc_ext.td
29 ↗(On Diff #297941)

alignment

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 updated this revision to Diff 297981.Oct 13 2020, 3:38 PM
michaelrj marked 14 inline comments as done.

Many small changes

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

michaelrj added inline comments.Oct 13 2020, 3:40 PM
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.

addressed comments.

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 added inline comments.Oct 13 2020, 9:05 PM
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 updated this revision to Diff 298240.Oct 14 2020, 2:34 PM

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 added inline comments.Oct 14 2020, 3:02 PM
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 added inline comments.Oct 14 2020, 3:18 PM
libc/spec/llvm_libc_ext.td
16 ↗(On Diff #298240)

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

michaelrj updated this revision to Diff 298265.Oct 14 2020, 5:11 PM
michaelrj marked 5 inline comments as done.

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 accepted this revision.Oct 14 2020, 10:33 PM

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.

This revision is now accepted and ready to land.Oct 14 2020, 10:33 PM

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

sivachandra retitled this revision from Move Cannonical Function List to [libc] Use entrypoints.txt as the single source of list of functions for a platform..Oct 14 2020, 10:37 PM
sivachandra edited the summary of this revision. (Show Details)
michaelrj marked 5 inline comments as done.

Fixed nits.