[libc-commits] [PATCH] D84848: [libc] Add a tool called WrapperGen.

Wed Jul 29 10:05:26 PDT 2020

sivachandra added a comment.

In D84848#2182387 <https://reviews.llvm.org/D84848#2182387>, @abrachet wrote:

> This looks good. I wonder though how we will deal with global variables. Those can't be easily wrapped like functions can.

Yes, global variables question is open if we have to support them in some manner. The one we have currently, `errno` which not really global but like one, is accessed by the extension function `__errno_location`. So, we are good in this case.  If there are other global variables we need to support (nothing comes to my mind), we can take that up at that time.

================
Comment at: libc/utils/tools/WrapperGen/Main.cpp:19-21
+llvm::cl::opt<std::string>
+    FunctionName("name", llvm::cl::desc("Name of the function to be wrapped."),
+                 llvm::cl::value_desc("<function name>"), llvm::cl::Required);
----------------
abrachet wrote:
> How do you imagine we will invoke this tool? Our list of function names is in TableGen so it wouldn't be easy to run this over every function name. Will this be called from `add_entrypoint_object` then?
Yes, the build rules will be the right place to hook this up. The downstream use case I am working with does exactly that.

================
Comment at: libc/utils/tools/WrapperGen/Main.cpp:58
+
+  // TODO: Arg types of the C++ implementation functions need not
+  // match the standard types. Either handle such differences here, or
----------------
abrachet wrote:
> I think this should be fine because all we care about it ABI compatibility, and we're only including the src/ headers not the include/ ones so the compiler wont complain there.
Right. In the existing code, there are no examples which break. But, added this `TODO` just in case. Does not add value you think?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84848