Page MenuHomePhabricator

[Clang][Sema] Detect section type conflicts between functions and variables
ClosedPublic

Authored by tmatheson on Dec 11 2020, 3:39 AM.

Details

Summary

If two variables are declared with attribute((section(name))) and
the implicit section types (e.g. read only vs writeable) conflict, an
error is raised. Extend this mechanism so that an error is raised if the
section type implied by a function's attribute((section)) conflicts
with that of another variable.

Diff Detail

Unit TestsFailed

TimeTest
15,900 msx64 debian > LLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-go go=/usr/bin/go test llvm.org/llvm/bindings/go/llvm
90 msx64 windows > LLVM.CodeGen/XCore::threads.ll
Script: -- : 'RUN: at line 1'; c:\ws\w32-1\llvm-project\premerge-checks\build\bin\llc.exe -march=xcore < C:\ws\w32-1\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll | c:\ws\w32-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w32-1\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll

Event Timeline

tmatheson created this revision.Dec 11 2020, 3:39 AM
tmatheson requested review of this revision.Dec 11 2020, 3:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 11 2020, 3:39 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aaron.ballman added inline comments.Dec 15 2020, 12:47 PM
clang/lib/Sema/SemaDeclAttr.cpp
3086

Does this need to be limited to function declarations? It seems to me that this should also be used on say ObjCMethod declarations or global variables as well, right?

If so, then Sema::UnifySection() may need to be updated as well as it currently accepts a DeclaratorDecl but some of these constructs (like ObjcMethodDecl and ObjCPropertyDecl) are NamedDecls and not declarators.

3087

NewAttr is already declared as being a SectionAttr, so this isn't needed, you can use NewAttr->getName() below instead.

clang/test/Sema/attr-section.c
42

Some other tests that may be interesting:

const int whatever1 __attribute__((section("seg4, sec4"))) = 10;
void test5(void) __attribute__((section("seg4, sec4")));
void test5(void) {}

void test6(void);
const int whatever2 __attribute__((section("seg5, sec5"))) = 10;
void test6(void) __attribute__((section("seg5, sec5")));
void test6(void) {}
tmatheson updated this revision to Diff 312232.Dec 16 2020, 9:12 AM

Extended to Objective-C methods and properties.
Added suggested C tests, C++ template function test and Objective-C tests.
I had to removed PSF_Implicit flag so that cases where the function was declared before the global variable were caught.

tmatheson marked 3 inline comments as done.Dec 16 2020, 9:36 AM
tmatheson added inline comments.
clang/lib/Sema/SemaDeclAttr.cpp
3086

I've extended this to include Objective-C methods and properties and template functions. Gobal variables are handled by Sema::CheckCompleteVariableDeclaration.

tmatheson marked an inline comment as done.
aaron.ballman accepted this revision.Dec 16 2020, 10:41 AM

LGTM aside from a minor nit. Thank you for the patch! Do you need me to commit on your behalf? If so, are you okay with Tomas Matheson <Tomas.Matheson@arm.com> for patch attribution?

clang/lib/Sema/SemaDeclAttr.cpp
3086–3087
This revision is now accepted and ready to land.Dec 16 2020, 10:41 AM
tmatheson updated this revision to Diff 312434.Dec 17 2020, 3:49 AM

Minor syntax change, fix CodeGen/attributes.c

tmatheson marked an inline comment as done.Dec 17 2020, 3:50 AM

LGTM aside from a minor nit. Thank you for the patch! Do you need me to commit on your behalf? If so, are you okay with Tomas Matheson <Tomas.Matheson@arm.com> for patch attribution?

Thanks for the review. I will ask for commit access for future but if you could commit this one in the meantime that would be appreciated. That's fine for attribution.

aaron.ballman closed this revision.Dec 17 2020, 8:44 AM

LGTM aside from a minor nit. Thank you for the patch! Do you need me to commit on your behalf? If so, are you okay with Tomas Matheson <Tomas.Matheson@arm.com> for patch attribution?

Thanks for the review. I will ask for commit access for future but if you could commit this one in the meantime that would be appreciated. That's fine for attribution.

I've commit on your behalf in f50066292477fb26806336e5604615d0eddde399, thank you for the patch!