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.
Details
Diff Detail
Event Timeline
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
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
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.
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. |
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.