Page MenuHomePhabricator

Add support for __declspec(code_seg("segname"))
ClosedPublic

Authored by Manna on Jul 2 2018, 10:09 AM.

Details

Summary

This patch uses CodeSegAttr to represent __declspec(code_seg) rather than building on the existing support for #pragma code_seg.
The code_seg declspec is applied on functions and classes. This attribute enables the placement of code into separate named segments, including compiler-generated codes and template instantiations.

For more information, please see the following:
https://msdn.microsoft.com/en-us/library/dn636922.aspx

This patch fixes the regression for the support for attribute ((section).
https://github.com/llvm-mirror/clang/commit/746b78de7812bc785fbb5207b788348040b23fa7

The regression was caused by previous MS __declspec(code_seg) support.
Patch by Soumi Manna (Manna)
https://reviews.llvm.org/D43352

The “declspec(code_seg)” uses CodeSegAttr. The patch is written to handle CodeGen support for the CodeSeg attribute. To prevent code_seg declspec interacting with attribute __((section)) or anything else that creates SectionAttr, the patch is written to check for SectionAttr (to check for #pragma code_seg related attributes) that only comes from #pragma code_seg and not a SectionAttr coming from some other construct that creates SectionAttrs.

Inside getImplicitCodeSegAttrFromClass routine, the patches are written to match with complicated Microsoft Compiler’s behavior (https://reviews.llvm.org/D22931, this is no longer available and it seems like Microsoft has removed this feedback page).

To match with the Microsoft Compiler, diagnostics messages are added for the code-seg
attribute mismatches on base and derived classes, virtual overrides, multiple and virtual inheritance, and function overloading.

Diff Detail

Repository
rL LLVM

Event Timeline

Manna created this revision.Jul 2 2018, 10:09 AM
Manna edited the summary of this revision. (Show Details)
Manna edited the summary of this revision. (Show Details)Jul 2 2018, 10:13 AM
Manna edited the summary of this revision. (Show Details)Jul 2 2018, 10:19 AM
Manna edited the summary of this revision. (Show Details)
Manna edited the summary of this revision. (Show Details)Jul 2 2018, 10:23 AM
Manna edited the summary of this revision. (Show Details)Jul 2 2018, 10:25 AM
Manna edited the summary of this revision. (Show Details)
Manna edited the summary of this revision. (Show Details)Jul 2 2018, 10:28 AM
Manna edited the summary of this revision. (Show Details)Jul 2 2018, 10:41 AM
majnemer added inline comments.Jul 3 2018, 3:46 PM
test/CodeGenCXX/code-seg.cpp
50–56 ↗(On Diff #153746)

I'd implement the MSVC compatible behavior if possible. The MSVC compiler team has taken the disposition of "whatever MSVC did with pragma code_seg/declspec(code_seg) is the right behavior". I'd also file a bug against them to fix their documentation.

Manna updated this revision to Diff 154231.Jul 5 2018, 7:16 AM
Manna retitled this revision from Support for __declspec(code_seg("segmentname")) to Add support for __declspec(code_seg("segname")).

The test is updated.

Manna marked an inline comment as done.Jul 5 2018, 7:35 AM
Manna added inline comments.
test/CodeGenCXX/code-seg.cpp
50–56 ↗(On Diff #153746)

Thanks for looking into the patches!

Inside getImplicitCodeSegAttrFromClass routine, the patch is written to match with Microsoft Compiler’s behavior for nested routines. I have updated this test to check for MSVC compatible behavior.

Please see “test/CodeGenCXX/code-seg1.cpp” for more test cases regarding the nested classes. All matches with MSVC compatible behavior. The MS compiler will apply a declspec from the parent class if there is no #pragma code_seg active at the class definition. If there is an active code_seg that is used instead.

A bug has been reported before.
Please see https://reviews.llvm.org/D22931, the Microsoft feedback page is no longer available and it seems like MS has removed the page.

From https://reviews.llvm.org/D22931
Thanks for contributing this! I actually worked on an implementation of this feature but never sent it out for review because I couldn't get an answer from Microsoft regarding this issue: https://connect.microsoft.com/VisualStudio/feedback/details/1203505/pragma-code-seg-and-declspec-code-seg-do-not-interact-correctly-with-nested-classes

aaron.ballman added inline comments.Jul 5 2018, 8:31 AM
lib/CodeGen/CodeGenModule.cpp
1391 ↗(On Diff #154231)

Can use const auto * here as the type is spelled out in the initialization.

1393 ↗(On Diff #154231)

Same.

1493–1495 ↗(On Diff #154231)

Same for these two as well.

lib/Sema/SemaDecl.cpp
2679 ↗(On Diff #154231)

Instead of rather doing a hasAttr<>() followed by a getAttr<>(), it would be better to hoist the getAttr<>() call out.

9256 ↗(On Diff #154231)

It's a bit strange that the function name says it's getting an implicit code_seg attribute but this is creating an implicit section attribute. Is it intentional that this function returns two different attributes depending on the function declaration?

lib/Sema/SemaDeclCXX.cpp
2251–2252 ↗(On Diff #154231)

These can be combined into a single if statement.

5629 ↗(On Diff #154231)

Extra space after Sema::

14640–14641 ↗(On Diff #154231)

These if statements can also be combined.

Manna updated this revision to Diff 154830.Jul 10 2018, 9:24 AM
Manna marked an inline comment as done.

Patch is updated.

Manna marked 8 inline comments as done.Jul 10 2018, 9:31 AM
Manna added inline comments.
lib/Sema/SemaDecl.cpp
9256 ↗(On Diff #154231)

Yes, it is intentional that this function returns two different attributes.

The function is renamed to getImplicitCodeSegOrSectionAttrForFunction() that would better describe the behavior.

aaron.ballman added inline comments.Jul 13 2018, 4:54 AM
lib/Sema/SemaDecl.cpp
8747 ↗(On Diff #154830)

Comment is now out of date as this could add a section attribute as well.

lib/Sema/SemaDeclAttr.cpp
2973 ↗(On Diff #154830)

This function is simple and only needs to be called once; why not sink it into the caller instead? If it needs to be factored into a separate function, I don't think it needs to be a member function of Sema.

Manna updated this revision to Diff 155702.Jul 16 2018, 9:23 AM
Manna marked an inline comment as done.
Manna marked 2 inline comments as done.Jul 16 2018, 9:27 AM
Manna added inline comments.
lib/Sema/SemaDecl.cpp
8747 ↗(On Diff #154830)

Comment is updated.

lib/Sema/SemaDeclAttr.cpp
2973 ↗(On Diff #154830)

+bool Sema::checkCodeSegName(SourceLocation LiteralLoc, StringRef CodeSegName) {

+ std::string Error = Context.getTargetInfo().isValidSectionSpecifier(CodeSegName);

This function is simple and only needs to be called once; why not sink it into the caller instead? If it needs to be factored into a separate function, I don't think it needs to be a member function of Sema.

This function is updated so that it will not be a member function of "Sema".

aaron.ballman accepted this revision.Jul 16 2018, 1:22 PM

The attribute bits look good to me, but I am less familiar with the behavioral bits so you may want to wait to see if @majnemer or @rnk have further comments on that.

This revision is now accepted and ready to land.Jul 16 2018, 1:22 PM
erichkeane requested changes to this revision.Jul 17 2018, 7:15 AM
erichkeane added inline comments.
include/clang/Sema/Sema.h
1955 ↗(On Diff #155702)

Since this is a new function, I suspect that the default parameter is a waste. We know all the places it is called, we can specify it.

lib/Sema/SemaDecl.cpp
2678 ↗(On Diff #155702)

Does this cover the situation where a re declaration creates a DIFFERENT code-seg? Those likely need to be checked, right?

lib/Sema/SemaDeclAttr.cpp
2913 ↗(On Diff #155702)

Did something get lost somewhere? This function is never referred to again.

This revision now requires changes to proceed.Jul 17 2018, 7:15 AM
Manna updated this revision to Diff 156103.Jul 18 2018, 10:01 AM
Manna marked 2 inline comments as done.

The patch is updated based on comment.

Manna marked 3 inline comments as done.Jul 18 2018, 10:34 AM
Manna added inline comments.
include/clang/Sema/Sema.h
1955 ↗(On Diff #155702)

Not sure what do you want to say here.

We don't need to specify all places it 'called since default parameter is used here.

Attr *getImplicitCodeSegOrSectionAttrForFunction(FunctionDecl *FD,

bool IsDefinition = true);

The compiler-generated and lambda causes would be true so the default argument is used there.

lib/Sema/SemaDecl.cpp
2678 ↗(On Diff #155702)

This is diagnosing the case where the New routine has a CodeSegAttr and the Old does not.

This code in mergeDeclAttributes here is added to handle the case of adding an explicit attribute when there was none on the class declaration.

Inside SemaDeclAttr.cpp file, CodeSegAttr *Sema::mergeCodeSegAttr covers the cases where different code-seg are created.

CodeSegAttr *Sema::mergeCodeSegAttr(Decl *D, SourceRange Range,

StringRef Name,
unsigned AttrSpellingListIndex) { }
lib/Sema/SemaDeclAttr.cpp
2913 ↗(On Diff #155702)

Yes, I changed this file and some of the codes got lost here accidently.

Sorry about it.

I have updated this function.

erichkeane added inline comments.Jul 18 2018, 10:54 AM
include/clang/Sema/Sema.h
1955 ↗(On Diff #155702)

I'm saying: The default parameter is only used in 2 places. Just specify it in those and remove the defaultness.

Manna updated this revision to Diff 156119.Jul 18 2018, 11:36 AM
Manna marked 3 inline comments as done.

patch is updated based on review comment.

Manna marked an inline comment as done.Jul 18 2018, 11:40 AM
Manna added inline comments.
include/clang/Sema/Sema.h
1955 ↗(On Diff #155702)

Thanks for the explanation. I have removed the default value and specified it .

erichkeane added inline comments.Jul 18 2018, 11:41 AM
lib/Sema/SemaDeclCXX.cpp
5636 ↗(On Diff #156119)

Don't define a variable for this, just do: /*IsDefinition=*/true

lib/Sema/SemaLambda.cpp
919 ↗(On Diff #156119)

Same here.

Manna updated this revision to Diff 156124.Jul 18 2018, 11:51 AM
Manna marked 3 inline comments as done.
erichkeane accepted this revision.Jul 18 2018, 11:55 AM

I've got a patch on deck in my environment, but I'll commit that when I'm done with it.

This revision is now accepted and ready to land.Jul 18 2018, 11:55 AM
This revision was automatically updated to reflect the committed changes.