This is an archive of the discontinued LLVM Phabricator instance.

Support `#pragma comment(lib, "name")` in the frontend for ELF
ClosedPublic

Authored by compnerd on Jan 31 2018, 12:22 PM.

Details

Reviewers
rnk
erichkeane
Summary

This adds the frontend support required to support the use of the comment pragma to enable auto linking on ELFish targets. This is a generic ELF extension supported by LLVM. We need to change the handling for the "dependentlib" in order to accommodate the previously discussed encoding for the dependent library descriptor. Without the custom handling of the PCK_Lib directive, the -l prefixed option would be encoded into the resulting object (which is treated as a frontend error).

Diff Detail

Repository
rC Clang

Event Timeline

compnerd created this revision.Jan 31 2018, 12:22 PM
compnerd updated this revision to Diff 132252.Jan 31 2018, 12:27 PM

Add missed file

ruiu added a comment.Jan 31 2018, 1:08 PM

#pragma comment is what Microsoft is using, but is everybody happy about that name? It isn't clear at least to me that what it actually means is linker directives.

I also wonder which is better #pragma comment(lib, "m") or #pragma comment(lib, "m").

ruiu added a comment.Jan 31 2018, 1:09 PM

I also wonder which is better #pragma comment(lib, "m") or #pragma comment(lib, "m").

Sorry, I meant #pragma comment(lib, "m") or #pragma comment("lib", "m").

In D42758#993936, @ruiu wrote:

I also wonder which is better #pragma comment(lib, "m") or #pragma comment(lib, "m").

Sorry, I meant #pragma comment(lib, "m") or #pragma comment("lib", "m").

I can't swear to it but I don't think Microsoft invented #pragma comment. Various IBM compilers have it, with a syntax of #pragma comment ( <keyword> [ , "string" ] ). I'm not seeing a lib keyword specifically in the IBM docs, but being a keyword would be consistent with past practice.

lib/Parse/ParsePragma.cpp
299

PS4's binary format is ELF, so you should be able to remove the isPS4 predicate.

382

Don't need isPS4, as it uses ELF.

compnerd marked 2 inline comments as done.Feb 2 2018, 10:53 AM

@probinson it would be pretty cool if we could get the PS4 environment to share the same linker options implementation. What other options do you guys need for this to be a viable approach?

This seems pretty OK to me. I'd like to see @probinson s PS4 discussion bottom out, but I don't see any reason to hold this up otherwise.

We definitely should leave the format as #pragma comment(keyword, "message"), since there is significant prior art here.

test/CodeGen/elf-linker-options.c
3 ↗(On Diff #132252)

I would like a test for Sema to validate the 'pragma ignored' warning in common cases (such as 'Linker').

compnerd updated this revision to Diff 132853.Feb 5 2018, 9:36 AM

clang-format missed line, simplify some checks

docs/LanguageExtensions.html should be updated to mention that we support this extension on all ELF targets.

I'd like to see @probinson s PS4 discussion bottom out, but I don't see any reason to hold this up otherwise.

The PS4 compiler uses its linker options mechanism to communicate three different things to the linker, namely #pragma comment(lib, ...) directives, as discussed here, symbols marked with __declspec(dllexport), and symbols marked with __declspec(dllimport) (the latter two requesting the linker explicitly to export/import the decorated symbols). I think if all three of these are converted to linker commands, we should be able to adopt the new implementation.

@jhenderson I believe that the first one is what this is implementing. I believe that adding the last two as a patch following this one is preferable as that is specific to the needs for PS4, but, both of those should be possible to accommodate. I would love to see a single unified approach here, so Im happy to help get that implemented.

compnerd updated this revision to Diff 133030.Feb 6 2018, 9:30 AM

Add additional test, update docs

erichkeane accepted this revision.Feb 6 2018, 9:34 AM

Just 1 format question, otherwise Looks good.

docs/LanguageExtensions.rst
2732 ↗(On Diff #133030)

The newlines in this section are seemingly inconsistent with what is above. Does that one violate the 80 char rule, or is this just short?

This revision is now accepted and ready to land.Feb 6 2018, 9:34 AM
compnerd added inline comments.Feb 6 2018, 4:54 PM
docs/LanguageExtensions.rst
2732 ↗(On Diff #133030)

Yeah, the previous section has 80-column violations :-(.

compnerd closed this revision.Feb 6 2018, 5:50 PM

SVN r324438