This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Treat auto-generated __dso_handle symbol as HiddenVisibility
ClosedPublic

Authored by mcgrathr on Feb 10 2017, 12:35 PM.

Diff Detail

Event Timeline

mcgrathr created this revision.Feb 10 2017, 12:35 PM

This is for clang, not llvm. Should it be cfe-commits instead of llvm-commits?

phosek edited edge metadata.Feb 10 2017, 3:16 PM
phosek edited subscribers, added: cfe-commits; removed: llvm-commits.

This is for clang, not llvm. Should it be cfe-commits instead of llvm-commits?

Right, my mistake.

mcgrathr updated this revision to Diff 88064.Feb 10 2017, 3:17 PM

Did you run all tests? I'd suspect that this might break test/OpenMP/threadprivate_codegen.cpp which is expecting @__dso_handle = external global i8 which will now be hidden global?

This was just the first crack, my first attempt at any change in LLVMland. I don't know how to run the tests yet.

You can use ninja check-clang to run Clang tests, if those pass you can use ninja check-all to run all tests. Testing guide has details about testing in LLVM.

mcgrathr updated this revision to Diff 88067.Feb 10 2017, 3:41 PM
mcgrathr updated this revision to Diff 88068.

This now passes check-clang.

phosek accepted this revision.Feb 10 2017, 4:44 PM

LGTM

This revision is now accepted and ready to land.Feb 10 2017, 4:44 PM
joerg requested changes to this revision.Feb 11 2017, 1:28 PM
joerg added a subscriber: joerg.
joerg added inline comments.
lib/CodeGen/CodeGenModule.cpp
2402

Can't this use the VarDecl argument of GetOrCreateLLVMGlobal to set the visibility? I'd prefer to keep reundant arguments to a minimum.

This revision now requires changes to proceed.Feb 11 2017, 1:28 PM
This revision was automatically updated to reflect the committed changes.
rnk added a subscriber: rnk.Feb 13 2017, 11:42 AM

I committed a modified patch that set the visibility on the global after creating it to avoid adding an optional boolean parameter. Thanks for the report and patch!