This is an archive of the discontinued LLVM Phabricator instance.

Emit section information for extern variables.
ClosedPublic

Authored by eandrews on Aug 8 2017, 2:05 PM.

Details

Summary

Currently, if _attribute_((section())) is used for extern variables, section information is not emitted in generated IR when the variables are used. This is expected since sections are not generated for external linkage objects. However NiosII requires this information as it uses special GP-relative accesses for any objects that use attribute section (.sdata). GCC keeps this attribute in middle-end.

This change emits the section information for all targets.

Diff Detail

Event Timeline

eandrews created this revision.Aug 8 2017, 2:05 PM

Please propose a patch for LangRef clarifying what exactly it means to have a section attribute on a declaration.

test/CodeGenCXX/extern-section-attribute.cpp
2

Not sure it really adds anything to run this with four different RUN lines.

efriedma added inline comments.Aug 8 2017, 2:20 PM
lib/CodeGen/CodeGenModule.cpp
2435

Why do you specifically check "D->hasExternalStorage() && !D->isThisDeclarationADefinition()", instead of just setting the section unconditionally?

eandrews added inline comments.Aug 8 2017, 2:39 PM
lib/CodeGen/CodeGenModule.cpp
2435

I noticed that you enter GetOrCreateLLVMGlobal( ) whenever the extern variable is declared as well as when it is defined. The flow of the program is different in each case. When the variable is defined, it also enters EmitGlobalVarDefinition( ). There is existing code handling section information here. I added the check in GetOrCreateLLVMGlobal( ) so the block gets skipped for variable definition, since its already handled elsewhere.

eandrews updated this revision to Diff 111064.Aug 14 2017, 1:52 PM

As per review comments, removed extra RUNS in lit test.

eandrews marked 3 inline comments as done.Aug 14 2017, 2:00 PM
erichkeane edited edge metadata.Aug 14 2017, 2:02 PM

@efriedma : LangRef changed in this review: https://reviews.llvm.org/D36712

LangRef has been updated and accepted. You can find it here - https://reviews.llvm.org/rL311459

Hey @efriedma : Did you get a chance to take a look at this? She got the langref changes in, so it is just waiting on this.

efriedma added inline comments.Aug 31 2017, 4:44 PM
lib/CodeGen/CodeGenModule.cpp
2435

I would rather just call setSection unconditionally here, as opposed to trying to guess whether the global will eventually be defined.

I'm also sort of concerned the behavior here could be weird if a section attribute is added on a redeclaration (e.g. what happens if you write extern int x; int y = &x; extern int x __attribute((section("foo")));)... maybe we should emit a warning?

eandrews updated this revision to Diff 116211.Sep 21 2017, 10:05 AM

I've updated the patch based on review comments. The changes include setting section unconditionally for extern variables and emitting a warning if section attribute is specified on a redeclaration.

eandrews marked an inline comment as done.Sep 21 2017, 10:05 AM
eandrews added inline comments.Sep 21 2017, 10:16 AM
lib/CodeGen/CodeGenModule.cpp
2435

@efriedma I modified the patch to emit a warning in the scenario you mentioned. A warning is also emitted for the following -

extern int x;  int *y=&x; int x __attribute__((section("foo")));

I thought it made sense to emit the warning for the latter as well. Should I restrict the warning to just redeclarations (without definition) instead?

Should I restrict the warning to just redeclarations (without definition) instead?

I think just redeclarations. The pattern of a declaration without a section followed by a definition with a section is common, and not really harmful.

lib/Sema/SemaDecl.cpp
2613

Probably don't want to check isUsed(); that's going to seem inconsistent to users.

eandrews updated this revision to Diff 116581.Sep 25 2017, 11:55 AM

I've modified the patch to emit a warning for re-declarations only. I also removed the isUsed check.

eandrews marked an inline comment as done.Sep 25 2017, 11:56 AM
This revision is now accepted and ready to land.Sep 26 2017, 3:55 PM
This revision was automatically updated to reflect the committed changes.