Page MenuHomePhabricator

Do not create LLVM IR `constant`s for objects with dynamic initialisation
ClosedPublic

Authored by chill on May 18 2021, 8:26 AM.

Details

Summary

When a const-qualified object has a section attribute, that
section is set to read-only and clang outputs a LLVM IR constant
for that object. This is incorrect for dynamically initialised
objects.

For example:

int init() { return 15; }
    
__attribute__((section("SA")))
const int a = init();

a is allocated to a read-only section and is left
unintialised (zero-initialised).

This patch adds checks if an initialiser is a constant expression
and allocates objects to sections as follows:

  • const-qualified objects
    • no initialiser or constant initialiser: .rodata
    • dynamic initializer: .bss
  • non const-qualified objects
    • no initialiser or dynamic initialiser: .bss
    • constant initializer: .data

(".rodata", ".data", and ".bss" names used just for explanatory
purpose)

Diff Detail

Event Timeline

chill requested review of this revision.May 18 2021, 8:26 AM
chill created this revision.

Once test accidentally copied to the wrong directory, I'm going to fix that.

rnk edited reviewers, added: hans; removed: rnk.May 20 2021, 10:48 AM
rnk removed a reviewer: whunt.
rnk added a subscriber: rnk.

@hans, do you mind reviewing this?

hans accepted this revision.May 21 2021, 1:45 AM

Looks great to me!

This revision is now accepted and ready to land.May 21 2021, 1:45 AM
chill updated this revision to Diff 347389.Mon, May 24, 7:34 AM

Moved a test from here to there, and did a few test tweaks, NFC.

Herald added a project: Restricted Project. · View Herald TranscriptMon, May 24, 2:05 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This breaks tests on macOS: http://45.33.8.238/macm1/10125/step_7.txt

Please take a look and revert for now if it takes a while to fix.

haowei added a subscriber: haowei.Mon, May 24, 4:45 PM

This CL breaks our mac builders as well. Could you either fix the test or revert the change please?

Error message:

******************** TEST 'Clang :: CodeGenCXX/const-dynamic-init.cpp' FAILED ********************
Script:
--
: 'RUN: at line 1';   /opt/staging/llvm_build/bin/clang -cc1 -internal-isystem /opt/staging/llvm_build/lib/clang/13.0.0/include -nostdsysteminc -triple x86_64-apple-darwin19.6.0 -emit-llvm -o - /opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp | /opt/staging/llvm_build/bin/FileCheck /opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp
--
Exit Code: 2

Command Output (stderr):
--
/opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp:3:24: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma
__attribute__((section("A")))
                       ^
/opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp:9:24: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma
__attribute__((section("B")))
                       ^
/opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp:15:24: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma
__attribute__((section("C")))
                       ^
/opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp:19:24: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma
__attribute__((section("D")))
                       ^
/opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp:23:24: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma
__attribute__((section("E")))
                       ^
5 errors generated.
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /opt/staging/llvm_build/bin/FileCheck /opt/llvm-project/clang/test/CodeGenCXX/const-dynamic-init.cpp

--

********************
chill added a comment.Tue, May 25, 1:29 AM

Thanks, I'll have a look.

chill reopened this revision.Tue, May 25, 1:29 AM
This revision is now accepted and ready to land.Tue, May 25, 1:29 AM
chill planned changes to this revision.Tue, May 25, 1:29 AM
chill updated this revision to Diff 347631.Tue, May 25, 4:26 AM

Updated a test to run for x86_64-linux instead of %itanium_abi_triple, to avoid having invalid
syntax for MACH-O sections. The patch itself does not care about section attribute syntax and x86 target
does not even need to be compiled.

This revision is now accepted and ready to land.Tue, May 25, 4:26 AM
hans accepted this revision.Tue, May 25, 4:40 AM
This revision was landed with ongoing or failed builds.Tue, May 25, 7:55 AM
This revision was automatically updated to reflect the committed changes.