Add option '-fvisibility=<hidden|default>' able to hide symbols not marked as 'export' on non-Windows targets. Resolves #2431 #2894

MrSmith33 · 2018-11-02T17:10:13Z

…ort' on linux and OSX. Resolves ldc-developers#2431.

thewilsonator

This needs a test case, but otherwise LGTM.

thewilsonator · 2018-11-03T10:41:14Z

dmd/globals.d

@@ -262,6 +262,9 @@ struct Param
        bool fullyQualifiedObjectFiles;
        bool cleanupObjectFiles;

+        // Export only symbols marked as 'export' on linux and OSX


Also add a note that this is the default behaviour on windows

thewilsonator · 2018-11-03T10:41:31Z

dmd/globals.h

@@ -240,6 +240,9 @@ struct Param
    bool fullyQualifiedObjectFiles;
    bool cleanupObjectFiles;

+    // Export only symbols marked as 'export' on linux and OSX


thewilsonator · 2018-11-03T10:41:52Z

driver/cl_options.cpp

@@ -81,6 +81,11 @@ static cl::opt<bool, true>
    createSharedLib("shared", cl::desc("Create shared library (DLL)"),
                    cl::ZeroOrMore, cl::location(global.params.dll));

+static cl::opt<bool, true> exportOnlyMarkedExport(
+        "export-marked-symbols", cl::ZeroOrMore,
+        cl::desc("Export only symbols marked as 'export' on linux and OSX"),


The description should say that it only affects non-Windows targets (as it's enforced for Windows, not just the default, and doesn't just apply to Linux and Mac).

thewilsonator · 2018-11-03T10:42:17Z

driver/main.cpp

@@ -558,6 +558,11 @@ void parseCommandLine(int argc, char **argv, Strings &sourceFiles,

  global.params.hdrStripPlainFunctions = !opts::hdrKeepAllBodies;
  global.params.disableRedZone = opts::disableRedZone();
+
+  // On windows this is already a default behavior


Nit already the default behavior

JohanEngelen · 2018-11-03T12:03:30Z

dmd/globals.d

@@ -262,6 +262,9 @@ struct Param
        bool fullyQualifiedObjectFiles;
        bool cleanupObjectFiles;

+        // Export only symbols marked as 'export' on linux and OSX
+        bool export_only_symbols_marked_export;


please use lowerCamelCase such that it matches our coding style.

I'd rather not have the flag in the Param struct at all; it's a pure codegen flag and doesn't affect the frontend.

And if it is there it should be in version(IN_LLVM).

JohanEngelen · 2018-11-03T12:06:45Z

driver/main.cpp

@@ -558,6 +558,11 @@ void parseCommandLine(int argc, char **argv, Strings &sourceFiles,

  global.params.hdrStripPlainFunctions = !opts::hdrKeepAllBodies;
  global.params.disableRedZone = opts::disableRedZone();
+
+  // On windows this is already a default behavior
+  if (global.params.isWindows) {


This may bite us in the future. Better to have the option mean what the behaviour will look like to the user. If Windows already does this by default, we should not internally change this option to false (which would mean the opposite would happen!), but instead change the way this option is used to achieve that behaviour.

+1. I'd call it internally something like defaultToHiddenVisibility; the gcc/clang option is -fvisibility=hidden, so our cmdline option could be named similarly.

JohanEngelen · 2018-11-03T12:07:31Z

gen/declarations.cpp

+        // Hide non-exported symbols
+        if (global.params.export_only_symbols_marked_export &&
+          !decl->isExport()) {
+          gvar->setVisibility(LLGlobalValue::HiddenVisibility);


If this is causing trouble on windows, then the check for windows should be here, instead of setting the option to false for Windows.

We use the DLL storage classes for Windows; I guess the visibility doesn't really matter.

kinke · 2018-11-03T13:43:37Z

This basic approach is more or less a port of the current Windows approach, i.e., allowing selective exports of functions and globals.

It doesn't handle any of the special symbols (TypeInfos, vtables, init symbols...); only exporting those for aggregates marked with export would be a follow-up we shouldn't forget.

MrSmith33 · 2018-11-03T17:06:12Z

What is the correct directory for this test?
Is it ok to use external tools for testing? (namely nm utility to dump exported symbols)
// RUN: ldc2 %s.d -betterC -shared -export-marked-symbols && nm lib%s.so

…port. State non-Windows targets instead of OSX and linux. Don't switch off the option on Windows, since it doesn't have effect anyway.

kinke · 2018-11-03T18:11:57Z

tests/codegen. There's an export.d already, testing the Windows dll{ex,im}port stuff (testing IR, not binary code).
nm is okay, as long as it works for CI for both Linux and Mac. Use XFAIL: Windows as Windows workaround.
Pls move the flag out of Param and expose it in driver/cl_options.h.

…portOnlySymbolsMarkedExport directly) Add tests for -export-marked-symbols switch.

JohanEngelen · 2018-11-03T23:12:45Z

tests/codegen/export_marked_symbols_thin_lto.d

@@ -0,0 +1,13 @@
+// Test if -export-marked-symbols works with thin LTO
+
+// RUN: ldc2 %S/inputs/export_marked_symbols_thin_lto_lib.d -c -export-marked-symbols -of=%t1.o


This will need a // REQUIRES: LTO and -flto=thin

JohanEngelen · 2018-11-03T23:15:08Z

tests/codegen/export_marked_symbols1.d

+// RUN: ldc2 %s -betterC -shared -export-marked-symbols -of=lib%t.so
+// RUN: nm lib%t.so | FileCheck %s
+
+// UNSUPPORTED: Windows


Nit: can you move this to the top? It's how we do it in other test files (requirements first), and it took me a while to see that you disabled this test on Windows. thanks! (also in the other test file)

…UIRES: LTO.

kinke · 2018-11-04T02:17:18Z

Thanks for figuring out the UNSUPPORTED on your own, I forgot we had it. ;)

For Mac, nm apparently needs -g (which should work on Linux too) to only display the public symbols. [I guess the linker strips the unused hidden ones on Linux via implicit --gc-sections, while the Mac linker seems to require LDC's -O to do the stripping.]

kinke · 2018-11-04T02:29:33Z

tests/codegen/export_marked_symbols2.d

+// CHECK: test__nonExportedFunDef
+// CHECK: test__nonExportedVarDef
+// CHECK-NOT: test__nonExportedFunDecl
+// CHECK-NOT: test__nonExportedVarDecl


The problem here is that you're only checking that the 2 latter symbols don't exist in the nm output after test__nonExportedVarDef. I don't think replacing CHECK with CHECK-DAG helps in that case; see https://llvm.org/docs/CommandGuide/FileCheck.html.
But apparently nm sorts alphabetically by default, so the list here should too.

…dows object extensions.

kinke · 2018-11-04T18:37:10Z

Thx dude, lgtm now. There's only the bikeshedding about the name of the cmdline option - I'm still in favor of gcc/clang-compatible -fvisibility=<hidden|default> and internally calling the flag defaultToHiddenVisibility. Interested in hearing other opinions.

JohanEngelen · 2018-11-04T19:18:08Z

terminology-wise it is smartest to use what others know about. LLVM and C seem to call the attribute "hidden visibility", so that'd have my vote too. (Also for buildsystems, the more LDC looks like a C compiler, the better)

edit: I think -fvisibility would also match GDC (!!!)

MrSmith33 · 2018-11-04T19:34:46Z

Sounds good. Would flto and useDeprecated option implementations be used as a guide?

kinke · 2018-11-04T20:40:19Z

Something like:

cl::opt<bool> defaultToHiddenVisibility(
    "fvisibility", cl::ZeroOrMore, cl::desc("Default visibility of symbols (not relevant for Windows)"),
    clEnumValues(
        clEnumValN(false, "default", "Export all symbols"),
        clEnumValN(true, "hidden", "Only export symbols marked with 'export'")
    ));

…bility'. Rename flag 'export-marked-symbols' to 'fvisibility' with 'default' and 'hidden' values.

MrSmith33 · 2018-11-04T21:30:46Z

Somehow clEnumValues doesn't work for opt

kinke · 2018-11-04T21:56:37Z

[I added a fixup commit, opt<ubyte> does the trick.]

MrSmith33 · 2018-11-08T10:10:17Z

@kinke What is the plan on this PR?

kinke · 2018-11-08T10:44:04Z

It'll be part of v1.13, don't worry. ;)

p0nce · 2018-11-08T12:08:45Z

Very happy about this outcome :)

patrickkh7788 · 2018-11-22T02:56:59Z

@p0nce
I try use -fvisibility=hidden with non-shared binary, it seems not working (test on mac-osx)?

I am not sure it should work for non-shared binary, but if so we can made binary more small.

Add option 'export-marked-symbols' to hide symbols not marked as 'exp…

f534ea4

…ort' on linux and OSX. Resolves ldc-developers#2431.

thewilsonator reviewed Nov 3, 2018

View reviewed changes

JohanEngelen reviewed Nov 3, 2018

View reviewed changes

Rename export_only_symbols_marked_export to exportOnlySymbolsMarkedEx…

2963c87

…port. State non-Windows targets instead of OSX and linux. Don't switch off the option on Windows, since it doesn't have effect anyway.

Remove exportOnlySymbolsMarkedExport from global Param. (Use opts::ex…

b1c72e2

…portOnlySymbolsMarkedExport directly) Add tests for -export-marked-symbols switch.

JohanEngelen reviewed Nov 3, 2018

View reviewed changes

Move UNSUPPORTED to the top of tests. Add missing -flto=thin. Add REQ…

3241688

…UIRES: LTO.

kinke reviewed Nov 4, 2018

View reviewed changes

Fix test check order (nm outputs in alphabetical order). Fix test Win…

8e6a535

…dows object extensions.

MrSmith33 changed the title ~~Add option 'export-marked-symbols' to hide symbols not marked as 'export' on linux and OSX. Resolves #2431~~ Add option 'export-marked-symbols' to hide symbols not marked as 'export' on non-Windows targets. Resolves #2431 Nov 4, 2018

Rename option 'exportOnlySymbolsMarkedExport' to 'defaultToHiddenVisi…

267ce97

…bility'. Rename flag 'export-marked-symbols' to 'fvisibility' with 'default' and 'hidden' values.

Fixup

348767a

Fixup header too...

d0eb1af

MrSmith33 changed the title ~~Add option 'export-marked-symbols' to hide symbols not marked as 'export' on non-Windows targets. Resolves #2431~~ Add option '-fvisibility=<hidden|default>' able to hide symbols not marked as 'export' on non-Windows targets. Resolves #2431 Nov 6, 2018

kinke merged commit 7bcd6d3 into ldc-developers:master Nov 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option '-fvisibility=<hidden|default>' able to hide symbols not marked as 'export' on non-Windows targets. Resolves #2431 #2894

Add option '-fvisibility=<hidden|default>' able to hide symbols not marked as 'export' on non-Windows targets. Resolves #2431 #2894

MrSmith33 commented Nov 2, 2018

thewilsonator left a comment

thewilsonator Nov 3, 2018

thewilsonator Nov 3, 2018

thewilsonator Nov 3, 2018

kinke Nov 3, 2018

thewilsonator Nov 3, 2018

JohanEngelen Nov 3, 2018

kinke Nov 3, 2018

thewilsonator Nov 3, 2018

JohanEngelen Nov 3, 2018

kinke Nov 3, 2018

JohanEngelen Nov 3, 2018

kinke Nov 3, 2018

kinke commented Nov 3, 2018

MrSmith33 commented Nov 3, 2018

kinke commented Nov 3, 2018 •

edited

Loading

JohanEngelen Nov 3, 2018

JohanEngelen Nov 3, 2018

kinke commented Nov 4, 2018 •

edited

Loading

kinke Nov 4, 2018

kinke commented Nov 4, 2018

JohanEngelen commented Nov 4, 2018 •

edited

Loading

MrSmith33 commented Nov 4, 2018

kinke commented Nov 4, 2018 •

edited

Loading

MrSmith33 commented Nov 4, 2018

kinke commented Nov 4, 2018

MrSmith33 commented Nov 8, 2018

kinke commented Nov 8, 2018

p0nce commented Nov 8, 2018

patrickkh7788 commented Nov 22, 2018

		@@ -0,0 +1,13 @@
		// Test if -export-marked-symbols works with thin LTO

		// RUN: ldc2 %S/inputs/export_marked_symbols_thin_lto_lib.d -c -export-marked-symbols -of=%t1.o

Add option '-fvisibility=<hidden|default>' able to hide symbols not marked as 'export' on non-Windows targets. Resolves #2431 #2894

Add option '-fvisibility=<hidden|default>' able to hide symbols not marked as 'export' on non-Windows targets. Resolves #2431 #2894

Conversation

MrSmith33 commented Nov 2, 2018

thewilsonator left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke commented Nov 3, 2018

MrSmith33 commented Nov 3, 2018

kinke commented Nov 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke commented Nov 4, 2018 • edited Loading

Choose a reason for hiding this comment

kinke commented Nov 4, 2018

JohanEngelen commented Nov 4, 2018 • edited Loading

MrSmith33 commented Nov 4, 2018

kinke commented Nov 4, 2018 • edited Loading

MrSmith33 commented Nov 4, 2018

kinke commented Nov 4, 2018

MrSmith33 commented Nov 8, 2018

kinke commented Nov 8, 2018

p0nce commented Nov 8, 2018

patrickkh7788 commented Nov 22, 2018

kinke commented Nov 3, 2018 •

edited

Loading

kinke commented Nov 4, 2018 •

edited

Loading

JohanEngelen commented Nov 4, 2018 •

edited

Loading

kinke commented Nov 4, 2018 •

edited

Loading