This is an archive of the discontinued LLVM Phabricator instance.

Add streamexecutor-config
ClosedPublic

Authored by jhen on Sep 7 2016, 10:23 AM.

Details

Summary

Similar to llvm-config, gets command-line flags that are needed to build
applications linking against StreamExecutor.

Diff Detail

Event Timeline

jhen updated this revision to Diff 70558.Sep 7 2016, 10:23 AM
jhen retitled this revision from to Add streamexecutor-config.
jhen updated this object.
jhen added reviewers: jprice, jlebar.
jhen added a subscriber: parallel_libs-commits.
jlebar added inline comments.Sep 7 2016, 10:34 AM
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in
63

Can we scope the try/except around tokens.index?

88

Consider description=__doc__ and then just updating this file's docstring.

130

Nit, write to stderr (print('foo', file=sys.stderr))? (Actually not quite a nit because of how this program is going to be used -- ideally we wouldn't print anything other than flags to stdout.)

131

Exit with an error code? Same elsewhere.

162

Should we do something to tokenize the flags? (So that e.g. -Idir with spaces shows up correctly?)

jhen updated this revision to Diff 70579.Sep 7 2016, 11:39 AM
jhen marked 5 inline comments as done.
  • Respond to jlebar's comments
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in
130

Since I'm using sys.exit here now, I'm just passing the error message to sys.exit instead and that will print it to stderr as well as returning error code 1.

162

Great catch! I added explicit quotes around @CMAKE_INSTALL_PREFIX@ everywhere it is used. This made it slightly more complicated to check whether a token being added by SE was already present in the LLVM output, so I added a new function has_token to do the token escaping before doing the search.

As for the output of llvm-config, I'll just trust that llvm-config quotes things correctly.

jlebar added inline comments.Sep 7 2016, 11:44 AM
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in
155

How does @CMAKE_INSTALL_PREFIX@ work? That is, who's responsible for replacing it with an actual path? From the example in the docstring, it looks like nobody is?

jhen added inline comments.Sep 7 2016, 11:55 AM
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in
155

This is a cmake built-in variable that is replaced at build time when this file is configured.

The code in this patch that sets up this configuration is the cmake configure_file command in line 2 in streamexecutor/tools/streamexecutor-config/CMakeLists.txt.

jlebar added inline comments.Sep 7 2016, 11:56 AM
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in
155

This is a cmake built-in variable that is replaced at build time when this file is configured.

...wow. Okay.

What happens if the install prefix contains the ' character? Do we just not care?

jprice added inline comments.Sep 7 2016, 12:07 PM
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in
148

I wonder if it's worth automatically picking out the path to llvm-config using @LLVM_BINARY_DIR@ instead of relying on it being on PATH. Makes things slightly easier for the user, and reduces the risk that they have a different LLVM on their PATH to the one that SE was built against.

jhen updated this revision to Diff 70582.Sep 7 2016, 12:28 PM
  • ESCAPE_QUOTES for cmake configure_file
streamexecutor/tools/streamexecutor-config/streamexecutor-config.in
155

Oh, thanks for reminding me. I added the ESCAPE_QUOTES option to the cmake configure_file command. That should make sure any quotes in the variable are properly escaped.

jlebar edited edge metadata.Sep 7 2016, 12:40 PM

OK. The python code '\"' is equivalent to the string containing one character, double-quote. So I think this is good so long as ESCAPE_QUOTES also escapes backslashes as \\ (which we care about because Windows filenames contain backslashes).

jlebar accepted this revision.Sep 7 2016, 12:40 PM
jlebar edited edge metadata.
This revision is now accepted and ready to land.Sep 7 2016, 12:40 PM
jhen updated this revision to Diff 70603.Sep 7 2016, 2:56 PM
jhen marked an inline comment as done.
jhen edited edge metadata.
  • Use LLVM_BINARY_DIR to find llvm-config
jhen added a comment.Sep 7 2016, 2:56 PM

OK. The python code '\"' is equivalent to the string containing one character, double-quote. So I think this is good so long as ESCAPE_QUOTES also escapes backslashes as \\ (which we care about because Windows filenames contain backslashes).

It turns out that ESCAPE_QUOTES doesn't do the right thing for Python because it is only meant for C string literals. So, for example, it doesn't escape single quotes. So I removed the ESCAPE_QUOTES option for cmake configure_file and instead wrapped the substitution in triple double quotes. This will still break if the path contains triple double quotes, but I'm willing to let that happen.

Between this change and the change to using LLVM_BINARY_DIRECTORY for the llvm-config path, jlebar, I think you might want to take another quick glance to make sure things still look OK.

streamexecutor/tools/streamexecutor-config/streamexecutor-config.in
150

I think that's a good idea. I made that change and added another option to let the user override the llvm-config path if the configuration somehow goes wrong.

jprice edited edge metadata.Sep 7 2016, 3:13 PM

Maybe I'm doing something wrong, but the single-quotes wrapping the include/lib paths given by --cppflags and --ldflags are not working for me. The compiler fails to find the StreamExecutor headers and libraries, and warns that the library directory doesn't exist. It's almost as if the quotes are being taken as part of the paths.

If I manually remove those quotes from the streamexecutor-config script then it works fine. It also works if I manually copy the output into the compiler command-line, so maybe there's some weird issue with how the quotes are handled by the shell when substituting command output with $()?

Tried on both OS X and Ubuntu, with a trivial program that just includes StreamExecutor.h.

jhen added a comment.Sep 7 2016, 3:50 PM

Maybe I'm doing something wrong, but the single-quotes wrapping the include/lib paths given by --cppflags and --ldflags are not working for me. The compiler fails to find the StreamExecutor headers and libraries, and warns that the library directory doesn't exist. It's almost as if the quotes are being taken as part of the paths.

If I manually remove those quotes from the streamexecutor-config script then it works fine. It also works if I manually copy the output into the compiler command-line, so maybe there's some weird issue with how the quotes are handled by the shell when substituting command output with $()?

Tried on both OS X and Ubuntu, with a trivial program that just includes StreamExecutor.h.

I can verify that the quotes are breaking it, and reading through the shell docs shows that the shell doesn't process quotes after command substitution, so I guess the quotes are being passed as literal characters in those arguments. I have an idea of how to fix this, and I should have a patch posted soon.

jhen added a comment.Sep 7 2016, 4:41 PM

Well, it turns out that the current llvm-config does not handle directories with special characters. I built llvm with -DCMAKE_INSTALL_PREFIX="$HOME/private/dir 'with' spaces" and installed it there. Then when I ran llvm-config --cppflags I got the following output:

-I/usr/local/google/home/jhen/private/dir 'with' spaces/include   -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS

where we can see that the spaces and single quotes have not been escaped.

I think we should mimic the llvm-config behavior because otherwise there will be two separate ways that paths are formatted in the output, and if anyone ever does make a workaround for llvm-config, it won't work for streamexecutor-config.

So I would say that even though it's kind of pathetic, I think we should not try to escape special characters in our output either.

jhen updated this revision to Diff 70620.Sep 7 2016, 4:43 PM
jhen edited edge metadata.
  • Remove quotes around CMAKE_INSTALL_PREFIX
jlebar added a comment.Sep 7 2016, 4:53 PM

I think we should mimic the llvm-config behavior because otherwise there will be two separate ways that paths are formatted in the output, and if anyone ever does make a workaround for llvm-config, it won't work for streamexecutor-config.

Sounds good to me.

So I would say that even though it's kind of pathetic [...]

You can always put a passive-aggressive comment in our source. :)

jprice accepted this revision.Sep 7 2016, 4:55 PM
jprice edited edge metadata.
This revision was automatically updated to reflect the committed changes.