This is an archive of the discontinued LLVM Phabricator instance.

[MS-ABI] Allow #pragma section to choose for ZI data
AbandonedPublic

Authored by javed.absar on Feb 24 2017, 1:48 AM.

Details

Summary

This patch enables the msvc pragma section to choose whether zero initialized variables are to be placed in data_seg or bss_seg. The current
implementation ignores bss_seg directive for variables that are initialized (i.e. it does not check if the initialization is, in fact, to all zeros).
This patch now allows that. The variables are now placed in named bss_seg but only so If (a) the initialization is all zeros; AND (b) ''-falways-use-bss flag is set'. In other words, current functionality is not impacted and a useful feature is provided to users.

Diff Detail

Event Timeline

javed.absar created this revision.Feb 24 2017, 1:48 AM
rogfer01 added inline comments.Feb 27 2017, 5:08 AM
lib/Driver/Tools.cpp
5940

I think you mean "-falways-use-bss".

Please add a test that checks that the driver is forwarding to CC1 the right flag. You only have a CC1 test that cannot catch this bug.

lib/Sema/SemaDecl.cpp
10742–10743

This check wasn't in the original code. Can you explain why it is needed here?

If it is needed, maybe you want a test for this as well.

test/CodeGenCXX/zi-sections.cpp
40

Would it make sense a test that checks array fillers?

int ArrayWithFiller[10] = { 0 };
int ArrayWithFillerTwo[10] = { 0, 2 };
int ArrayWithAllFiller[10] = { };

Hi Roger:
Thanks for the review. I have extended the test and fixed driver flag passing that you pointed out.

Regarding the check in Sema::DeclarationIsZeroInitialized, the check if to keep the function return value consistent, in case function is called with a declaration that is without an initializer.
Best Regards,
Javed

rogfer01 edited edge metadata.Feb 28 2017, 2:21 AM

Thanks @javed.absar

This LGTM now.

thakis added a subscriber: thakis.Feb 28 2017, 8:20 AM

I'm surprised this is behind a flag. This pragma attempts to be compatible with cl.exe. Does cl.exe do this by default? If so, we should do it by default. If not, why add this as an incompatible thing to an MS extension instead of keying it off some attribute that works in non-MS mode too?

Hi Nico:
This is for the case when using '#pragma bss_seg..' feature outside of msvc context. That's the reason for the flag.
One could use attribute, but as attribute is attached to individual declaration, while pragma applies to whole file (unless superseded by another pragma), the latter is easier to use in some cases (i.e. when there are lots of such declarations). Hope this clarifies the question you asked.
Best Regards
Javed

thakis added a comment.Mar 1 2017, 8:40 AM

Adding incompatible extensions to MS extensions sounds like bad precedent to me. I think it'd be good if a code owner could opine on this change before it goes in.

rnk edited edge metadata.Mar 1 2017, 1:10 PM

This functionality doesn't feel worth a driver flag. It customizes a very small aspect of a Microsoft extension that was only implemented for compatibility. It also reimplements the logic that LLVM uses in the backend to put global variables in .bss or .data. Can you elaborate on the actual use case? Is the goal to put all globals that would normally live in .bss into a custom section? If so, I think we should implement our own pragma to do that rather than piggybacking on the Microsoft one.

lib/AST/Expr.cpp
2841

I seriously doubt we need this much code to check if something is a zero. I'm sure we already have a way to do this.

javed.absar abandoned this revision.Jun 22 2017, 9:40 AM

Abandoning as there is a separate pragma clang section implementation now.