Page MenuHomePhabricator

[CodeGen] Fix CodeGenModule::CreateGlobalInitOrDestructFunction
ClosedPublic

Authored by ahatanak on Oct 9 2015, 3:40 PM.

Details

Summary

This patch makes the following changes to CodeGenModule::CreateGlobalInitOrDestructFunction:

  • Use SetInternalFunctionAttributes instead of SetLLVMFunctionAttributes. Some of the attributes (e.g., stack protector strong) are added in SetLLVMFunctionAttributesForDefinition, which gets called from SetInternalFunctionAttributes.
  • Pass the correct CGFunctionInfo to SetInternalFunctionAttributes instead of always passing what arrangeNullaryFunction returns. arrangeNullaryFunction returns the CGFunctionInfo for functions of type void(), but some of the functions are of different types. I haven't found a case where clang mis-compiles because of this bug, but nevertheless I think we should pass the correct type here.

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak updated this revision to Diff 37001.Oct 9 2015, 3:40 PM
ahatanak retitled this revision from to [CodeGen] Fix CodeGenModule::CreateGlobalInitOrDestructFunction.
ahatanak updated this object.
ahatanak added a reviewer: dexonsmith.
ahatanak added a reviewer: echristo.
ahatanak added a subscriber: cfe-commits.

Add a comment to send this patch to cfe-commits.

echristo edited edge metadata.Oct 21 2015, 1:00 PM

Can you give me some more background here? I'm not sure I see what's wrong and what's going wrong.

Thanks!

-eric

The patch isn't fixing any serious bugs, but is fixing what seems to me inconsistencies in the code. It makes the following changes:

  1. Currently, SetLLVMFunctionAttributes is called to add function attributes to the internal function definitions, but SetLLVMFunctionAttributes doesn't call SetLLVMFunctionAttributesForDefinition which should be called to add function attributes to function definitions (but not to declarations). This patch replaces calls to SetLLVMFunctionAttributes with calls to SetInternalFunctionAttributes, which I think is the right thing to do since the functions created are all internal functions.
  1. Currently, CodeGenModule::CreateGlobalInitOrDestructFunction always passes a CGFunctionInfo instance for void() functions (created by arrangeNullaryFunction) to SetLLVMFunctionAttributes. Strictly speaking, this is not correct for CodeGenFunction::generateDestroyHelper (near line 582) because the function type is void(*void.). That's why I made changes to have the callers of CreateGlobalInitOrDestructFunction pass a reference to CGFunctionInfo.
echristo accepted this revision.Oct 29 2015, 1:42 PM
echristo edited edge metadata.

LGTM.

Thanks!

-eric

This revision is now accepted and ready to land.Oct 29 2015, 1:42 PM
This revision was automatically updated to reflect the committed changes.

Thanks for this. I was about to create the same patch (after seeing this broken on 3.7) only to see it had already been done!