[cmake-developers] Adding logic to CMake for -fPIE and -fPIC

[Stephen Kelly]

Hi,

I've re-pushed to my gitorious clone.

Brad King wrote:

> On 05/17/2012 08:36 AM, Stephen Kelly wrote:
>> Done and re-pushed to my clone. I still have to write the unit tests, but
>> I think it can be reviewed again at this point.
> 
> Great start.  Here are some comments.
> 
> The "Update the documentation of IMPORTED_LOCATION to match the code."
> commit
> has an incorrect message.  You're modifying the
> MAP_IMPORTED_CONFIG_<CONFIG>
> docs.  Also, the original wording the commit changes is correct under the
> assumption that the property is set, and if the property is not set then
> why
> should its docs apply?  Anyway, I think it can be made even more clear
> than your proposed wording by using
> 
>   "If this property is set and no matching configurations..."

Merged to next as a separate branch.

> 
> -----
> 
> The LanguageToCachedSharedLibFlags ivar would be better named
> LanguageToOriginalSharedLibFlags IMO.  Also please add a comment
> where they are set to explain why (just ref the policy).

Done.

> 
> The proposed CMP0018 summary should prefer "POSITION_INDEPENDENT_CODE"
> instead of "COMPILE_OPTIONS", no?  The property is part of the public
> interface the user can set/get.  The COMPILE_OPTIONS values are impl
> details.  Also the documentation appears to be cut-n-pasted from another
> policy.  Please fill in the details.

Done. Looks like I missed step 3 or 'copy/paste/modify' :)

> 
> The docs of POSITION_INDEPENDENT_CODE are incorrect.  The last sentence
> should be "This property is true by default for SHARED and MODULE library
> targets and false otherwise.".

Done.

> 
> Your logic should use GetSafeDefinition instead of GetDefinition before
> storing the result in a std::string because the latter returns NULL if
> not set.

Given that the variable being replaced is fetched as a RequiredDefinition, I 
used that for now. I guess I can change it, but I think it makes sense as a 
RequiredDefinition.

> 
> AddPositionIndependentFlags shouldn't care whether the library is
> shared or not, right?

My vague understanding up to now has been that static libraries should not 
get the flag:

http://www.gnu.org/software/libtool/manual/html_node/Static-libraries.html
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28811
http://stackoverflow.com/questions/1589977/linking-a-shared-library-against-
a-static-library-must-the-static-library-be-co

I'm not certain though...

> If the property is true then we want the flags.
> Perhaps:
> 
>  std::string picFlags;
>  if (targetType == cmTarget::EXECUTABLE)
>    {
>    std::string flagsVar = "CMAKE_";
>    flagsVar += lang;
>    flagsVar += "_COMPILE_OPTIONS_PIE";
>    picFlags = this->Makefile->GetSafeDefinition(flagsVar.c_str());
>    }
>  if(picFlags.empty())
>    {
>    std::string flagsVar = "CMAKE_";
>    flagsVar += lang;
>    flagsVar += "_COMPILE_OPTIONS_PIC";
>    picFlags = this->Makefile->GetSafeDefinition(flagsVar.c_str());
>    }
>  if(!picFlags.empty())
>    {
>    this->AppendFlags(flags, picFlags.c_str());
>    }

Done, though I don't know if static libraries should be excluded here.

> 
> There is a lot more to adding a policy than just
> 
>   GetPolicyStatus(cmPolicies::CMP0018) == cmPolicies::OLD
> 
> Look at some of the other policies.  You need to handle all the possible
> values.  If it is WARN then you need to produce the warning and treat as
> OLD behavior.  See the switch() statements used for others.

Done.

I'm also not completely certain about the change to the try_compile behavior 
in the branch. Could there be any reason to use 
set(CMAKE_POSITION_INDEPENDENT_CODE ON) but want to do a try_compile without 
-fPI{C,E} ? My guess is no, because one would do the try_compile before 
conditionally setting that property, I guess.

Thanks,

Steve.