Use Whitequark's Code to extract #define's and other compile time info from object files #678

rgrinberg · 2018-04-06T03:47:25Z

note: only the last commit is relevant to the PR. Here's the commit message:

Do not run compiled programs to extract #define's

Running a program to extract a #define value doesn't work in a cross compilation
environment. Nevertheless, we can extract #define constants by invoking the
preprocessor directly using the -E flag and doing some parsing to extract
values.

As a consequence, we now ignore the link_flags argument. As we're not going to
be linking any executables. We aren't removing the argument altogether since
it's technically a breaking change. The user will instead see a deprecation
warning when ~link_flags is provided.

@dra27 would be good to hear your opinion about this and how well would this hack work on Windows.

cc @Chris00 because I know he was interested in implementing this in the original configurator

cc @avsm b/c it's a general cross compilation issue

I know that @whitequark implemented a solution to the same problem in ctypes. But he opted for simply compiling the programs and then using preprocessor hacks in conjunction with regex to extract the #define's. Would be good to have his opinion here about this approach instead.

whitequark · 2018-04-06T03:57:17Z

I know that @whitequark implemented a solution to the same problem in ctypes. But he opted for simply compiling the programs and then using preprocessor hacks in conjunction with regex to extract the #define's. Would be good to have his opinion here about this approach instead.

In case of ctypes the problem was more complex as the values that I needed weren't just hidden in preprocessor definitions but they were the result of evaluating sizeof. So, nothing short of actual compilation would have (reasonably portably) worked.

Also, she*.

rgrinberg · 2018-04-06T05:46:39Z

On Fri, Apr 6, 2018, 11:57 AM whitequark ***@***.***> wrote: In case of ctypes the problem was more complex as the values that I needed weren't just hidden in preprocessor definitions but they were the result of evaluating sizeof. So, nothing short of actual compilation would have (reasonably portably) worked.

Hmm, a cross compilation solution that doesn't support ctypes is certainly not satisfactory to us. How do you feel about us just taking your code and incorporating it into configurator? I don't see any other way to support this otherwise. I also wonder if we should keep this preprocessor appoach as an optimization or as backup.

Also, she*.

Noted. Thanks.

…

whitequark · 2018-04-06T05:50:56Z

Hmm, a cross compilation solution that doesn't support ctypes is certainly
not satisfactory to us. How do you feel about us just taking your code and
incorporating it into configurator? I don't see any other way to support
this otherwise.

I hereby declare any contributions I made for the ctypes build system licensed under CC0, so go ahead and take them.

I also wonder if we should keep this preprocessor appoach as an
optimization or as backup.

The preprocessor approach is clearly cleaner, but is strictly less general, so I'm not sure if much can be gained from it. One thing it might be useful for is getting a list of all definitions, which is much faster.

rgrinberg · 2018-04-06T06:53:09Z

The preprocessor approach is clearly cleaner, but is strictly less general, so I'm not sure if much can be gained from it. One thing it might be useful for is getting a list of all definitions, which is much faster.

Well I was thinking about possible portability issues. But this is probably just paranoia and me being pretty unfamiliar with this subject. I suppose we can keep the -E solution in the git history until we have a solid use case for it.

rgrinberg · 2018-04-06T07:18:04Z

@whitequark quick question regarding Extract_from_c: there doesn't seem to have a way to check if a constant is defined (the Switch check . I could probably figure this out, but if you know something off the top of your head, then please shoot.

rgrinberg · 2018-04-06T07:22:47Z

Ah, I just remembered that a CLA is necessary before we include your code.

https://janestreet.github.io/contributing.html

Sorry for the hassle, I know this is a drag.

whitequark · 2018-04-06T07:26:46Z

"Fax"? What century is this?

Anyway, it's going to be a while until I have any access to a printer and scanner, so you'll have to wait, I guess.

whitequark · 2018-04-06T07:30:56Z

quick question regarding Extract_from_c: there doesn't seem to have a way to check if a constant is defined (the Switch check . I could probably figure this out, but if you know something off the top of your head, then please shoot.

Sure. Insert into the usual template:

#ifdef %s
const char *s = "BEGIN-Y-END";
#else
const char *s = "BEGIN-N-END";
#endif

rgrinberg · 2018-04-06T07:55:35Z

Okay, thanks for the quick response. If you have a smart phone then a scanner is likely not necessary btw 😅

There was talk about simplifying the CLA into a DCO. Any updates on that @avsm @yminsky?

whitequark · 2018-04-06T08:07:31Z

There was talk about simplifying the CLA into a DCO.

Why not simplify the CLA signing process instead? E.g. with clahub.

rgrinberg · 2018-04-06T13:06:26Z

Given that we want to keep this approach in the git history (in case it ever comes in handy), the plan is to merge this PR as is (after review). I will prepare a new PR to use the extract_from_c approach.

rgrinberg · 2018-04-11T03:27:00Z

Btw, we have 1 more reason to merge this PR in its current form. We don't want to delay things until whitequark signs the CLA. And we don't wanna rush her either.

Though I'm not yet sure if we'll include this feature in 1.21 - which will be the first release with jbuilder.configurator.

Chris00 · 2018-04-11T09:57:34Z

I think @whitequark 's approach is very nice and suits my needs — while the -E approach does not. IMHO, it should be generalized in a straightforward manner to require a single compilation to extract multiple values (this should make the extra cost reasonable).

ghost · 2018-04-11T11:44:57Z

Unless there is a need for the current PR right away, I think we should wait and merge only the final version.

whitequark · 2018-04-11T13:24:09Z

Okay, I'll try to get CLA signed faster, given that it doesn't look like the situation with it will change. Will try to do it today.

ghost · 2018-04-11T15:26:47Z

Thanks @whitequark.

@rgrinberg regarding switching to a DCO, we are making good progress. I just had another meeting to discuss some of the questions, but it's not yet confirmed.

whitequark · 2018-04-11T22:14:22Z

CLA signed and sent to core-cla@.

ghost · 2018-04-12T09:41:39Z

Well received, thanks!

rgrinberg · 2018-04-13T13:03:55Z

Because of the swift turnaround in the CLA situation, we can just proceed and do everything in this PR.

So I implemented whitequark's method and made it work with a single compilation step. What remains is:

Replace use of Str. I guess I'll use ocamllex since using jbuilder's re is a bit painful
Test things on Windows.
Share code between compile_c_prog and compile_and_link_prog.

@Chris00 or @dra27 running the test on windows would be appreciated.

rgrinberg · 2018-04-14T13:20:10Z

@whitequark could you describe what the STRINGIFY macros is for? The definition is a bit mysterious:

#define STRINGIFY1(x) #x
#define STRINGIFY(x) STRINGIFY1(x)

whitequark · 2018-04-14T13:31:08Z

@rgrinberg https://stackoverflow.com/questions/16989730/stringification-how-does-it-work

rgrinberg · 2018-04-15T17:19:50Z

@whitequark thanks.

One thing that's left out of this PR is support for extracting the values of macros like STRINGIFY and other compile time information like alignof or sizeof. I've strictly implemented the old discovery API in terms of this new approach.

I will propose some ways to expose the new type of information plus the ability for a user to define their own macros in a follow up.

whitequark · 2018-04-15T17:41:08Z

src/configurator/v1.ml

@@ -329,8 +329,6 @@ module C_define = struct
    );
    if has_type Type.String then (
      pr {|
-#define STRINGIFY1(x) #x
-#define STRINGIFY(x) STRINGIFY1(x)

 #if __USE_MINGW_ANSI_STDIO && defined(__MINGW64__)
 #define REAL_ARCH_INTNAT_PRINTF_FORMAT \"ll\"


by the way this is internal to ctypes, you probably don't need it.

Thanks, I'll remove it. There will be a way to add and use your own custom macros so this use case will be covered. The end goal is to port ctypes to jbuilder, so if you see anything else that needs to be done for that happen, please let me know.

ghost · 2018-04-16T08:33:03Z

This is a nice method, I didn't know about it!

@rgrinberg, IIUC the code currently relies on the order of strings in the object file? This looks a bit fragile to me.

whitequark · 2018-04-16T08:34:34Z

Yes, it needs to emit strings like "BEGIN-0-" etc.

ghost · 2018-04-16T10:02:31Z

ok

Chris00

Looks good modulo the remark made that one should not rely on the order of the variables.

Chris00 · 2018-04-16T09:14:56Z

src/configurator/extract_obj.mli

@@ -0,0 +1 @@
+val extract : string list -> Lexing.lexbuf -> string list


Could you please document it? In particular, you may want to let the reader know that this need to be synchronized with...

Chris00 · 2018-04-16T11:30:48Z

src/configurator/v1.ml

+    Buffer.contents buf
+
+  let extract_values obj_file vars =
+    let obj = Io.read_file obj_file in


What is the motivation to read the whole file into memory? Granted, it is small so it is no harm but I am wondering...

Fair enough. I pushed a commit to fix this.

rgrinberg · 2018-04-16T17:51:33Z

I'm no longer relying on the order of strings in the object file. @diml have another look.

ghost · 2018-04-17T08:36:45Z

src/configurator/v1.ml

+
+  let extract_values obj_file vars =
+    let values =
+      Exn.protectx (open_in obj_file)


You can use Io.with_lexbuf_from_file here

rgrinberg · 2018-04-17T11:20:36Z

@diml good point. fixed.

I also added an entry module so that the helper modules isn't accessible to the user.

ghost · 2018-04-17T13:25:54Z

Ok, LGTM!

Running a program to extract a #define value doesn't work in a cross compilation environment. Nevertheless, we can extract #define constants by invoking the preprocessor directly using the -E flag and doing some parsing to extract values. As a consequence, we now ignore the link_flags argument. As we're not going to be linking any executables. We aren't removing the argument altogether since it's technically a breaking change. The user will instead see a deprecation warning when ~link_flags is provided.

Only expose the public api (v1) in it

rgrinberg requested review from a user, Chris00 and dra27 April 6, 2018 03:47

rgrinberg force-pushed the configurator-E branch from aae5ae7 to 921f578 Compare April 6, 2018 06:46

emillon mentioned this pull request Apr 6, 2018

Clarify the CLA situation #682

Closed

rgrinberg force-pushed the configurator-E branch from 921f578 to 120c608 Compare April 12, 2018 16:38

rgrinberg changed the title ~~Use -E flag to extract #define's~~ Use Whitequark's Code to extract #define's and other compile time info from object files Apr 13, 2018

rgrinberg mentioned this pull request Apr 13, 2018

Configurator Quark #683

Closed

rgrinberg closed this Apr 14, 2018

rgrinberg reopened this Apr 14, 2018

whitequark reviewed Apr 15, 2018

View reviewed changes

Chris00 approved these changes Apr 16, 2018

View reviewed changes

rgrinberg mentioned this pull request Apr 16, 2018

Add ?macros argument when import constants #704

Merged

rgrinberg force-pushed the configurator-E branch from cde5b30 to 9cf6ebf Compare April 16, 2018 17:45

Chris00 approved these changes Apr 16, 2018

View reviewed changes

ghost suggested changes Apr 17, 2018

View reviewed changes

rgrinberg force-pushed the configurator-E branch 2 times, most recently from 2bcfc11 to 4c2c7ba Compare April 17, 2018 11:53

rgrinberg added 3 commits April 17, 2018 22:24

Extract #define's using @whitequark's hack

1d33f81

Add entry module for configurator

3e1495e

Only expose the public api (v1) in it

rgrinberg force-pushed the configurator-E branch from 4c2c7ba to 3e1495e Compare April 17, 2018 15:25

rgrinberg merged commit cc8de7d into ocaml:master Apr 18, 2018

rgrinberg deleted the configurator-E branch April 18, 2018 06:40

ghost mentioned this pull request Jul 10, 2018

[new release] dune (1.0.0) ocaml/opam-repository#12323

Merged

		@@ -0,0 +1 @@
		val extract : string list -> Lexing.lexbuf -> string list

Use Whitequark's Code to extract #define's and other compile time info from object files #678

Use Whitequark's Code to extract #define's and other compile time info from object files #678

Conversation

rgrinberg commented Apr 6, 2018

note: only the last commit is relevant to the PR. Here's the commit message:

whitequark commented Apr 6, 2018

rgrinberg commented Apr 6, 2018 via email

whitequark commented Apr 6, 2018

rgrinberg commented Apr 6, 2018

rgrinberg commented Apr 6, 2018

rgrinberg commented Apr 6, 2018

whitequark commented Apr 6, 2018

whitequark commented Apr 6, 2018

rgrinberg commented Apr 6, 2018

whitequark commented Apr 6, 2018

rgrinberg commented Apr 6, 2018

rgrinberg commented Apr 11, 2018

Chris00 commented Apr 11, 2018

ghost commented Apr 11, 2018

whitequark commented Apr 11, 2018

ghost commented Apr 11, 2018

whitequark commented Apr 11, 2018

ghost commented Apr 12, 2018

rgrinberg commented Apr 13, 2018

rgrinberg commented Apr 14, 2018

whitequark commented Apr 14, 2018

rgrinberg commented Apr 15, 2018

whitequark Apr 15, 2018

Choose a reason for hiding this comment

rgrinberg Apr 16, 2018

Choose a reason for hiding this comment

ghost commented Apr 16, 2018

whitequark commented Apr 16, 2018

ghost commented Apr 16, 2018

Chris00 left a comment

Choose a reason for hiding this comment

Chris00 Apr 16, 2018

Choose a reason for hiding this comment

Chris00 Apr 16, 2018

Choose a reason for hiding this comment

rgrinberg Apr 16, 2018

Choose a reason for hiding this comment

rgrinberg commented Apr 16, 2018

ghost Apr 17, 2018

Choose a reason for hiding this comment

rgrinberg commented Apr 17, 2018

ghost commented Apr 17, 2018