Skip to content

Conversation

@uweigand
Copy link
Contributor

@uweigand uweigand commented Oct 5, 2021

This makes the Mono build of libcoreclr.so on Linux look like the CoreCLR build w.r.t. these two issues.

@ghost ghost added area-Build-mono community-contribution Indicates that the PR has been added by a community member labels Oct 5, 2021
Copy link
Member

@akoeplinger akoeplinger Oct 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this consistent with Android:

Suggested change
add_link_options(-Wl,-z,relro,-z,now)
add_compile_options(-Wl,-z,now)
add_compile_options(-Wl,-z,relro)

... and what about noexecstack?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I just saw #59987 (comment) and you tried that already. it's interesting why this doesn't work on Linux, can you please still split it into different lines and leave a comment about why add_link_options is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "add_compile_options" didn't actually work for me - it is not being applied at the final link step, which is actually the only step where it really matters.

As to noexecstack, the library is already marked as noexecstack to me. That's to be expected as the compilers already mark objects files they generate accordingly. The only reason for a manual override is when you link in a full assembler file (.s), which I don't believe happens in the Mono build.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, didn't see the latest comment. I'm actually a bit surprised that add_compile_options works for Android, I would have expected the same problem (I can't really test that here). If you prefer two lines, I can certainly change that, I just used what was in eng/native/configurecompiler.cmake ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comment and split back into two lines. Let me know if there's anything else you'd like me to do. Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good now. We can probably change Android to use add_link_options too, I suppose add_compile_options works because we use clang there.

@akoeplinger
Copy link
Member

@uweigand looks like there's an issue in the AOT cross compiler build on Windows:

  D:\workspace\_work\1\s\artifacts\obj\mono\Android.x64.Release\cross\NativeVersion.rc(6) : error RC2104 : undefined keyword or key name: VER_FILEVERSION

@uweigand
Copy link
Contributor Author

uweigand commented Oct 5, 2021

@uweigand looks like there's an issue in the AOT cross compiler build on Windows:

  D:\workspace\_work\1\s\artifacts\obj\mono\Android.x64.Release\cross\NativeVersion.rc(6) : error RC2104 : undefined keyword or key name: VER_FILEVERSION

Ah, right. Looks like this needs to check $(OS) instead of $(TargetOS). I'll fix.

@uweigand
Copy link
Contributor Author

uweigand commented Oct 5, 2021

The Browser_wasm_Windows failure looks related:

2021-10-05T16:38:59.7309497Z   CMake Error at mono/mini/CMakeLists.txt:326 (add_library):
2021-10-05T16:38:59.7310657Z     Cannot find source file:
2021-10-05T16:38:59.7312737Z   
2021-10-05T16:38:59.7313609Z       D:/workspace/_work/1/s/artifacts/obj/mono/Browser.wasm.Release/version.c

So I think this is yet another case, where we're cross-building a version of the runtime hosted on Wasm, right? We don't get a version.c because in this case $(OS) == 'Windows_NT'. And it's challenging to get a version.c because the GenerateNativeVersionFile target in arcade uses the same $(OS) == 'Windows_NT' check and would create the Windows version header contents anyway.

Also, it's not clear that using an embedded sccsid string would even work for a wasm/emscripten binary ... Should we just keep not doing any version string in the HOST_BROWSER case then?

* Link with -z relro -z now to enable full RELRO mode
  Fixes #59904

* Generate and link in version.c to provide version string
  Fixes dotnet/source-build#2484
@uweigand
Copy link
Contributor Author

uweigand commented Oct 5, 2021

The Browser_wasm_Windows failure looks related:

2021-10-05T16:38:59.7309497Z   CMake Error at mono/mini/CMakeLists.txt:326 (add_library):
2021-10-05T16:38:59.7310657Z     Cannot find source file:
2021-10-05T16:38:59.7312737Z   
2021-10-05T16:38:59.7313609Z       D:/workspace/_work/1/s/artifacts/obj/mono/Browser.wasm.Release/version.c

So I think this is yet another case, where we're cross-building a version of the runtime hosted on Wasm, right? We don't get a version.c because in this case $(OS) == 'Windows_NT'. And it's challenging to get a version.c because the GenerateNativeVersionFile target in arcade uses the same $(OS) == 'Windows_NT' check and would create the Windows version header contents anyway.

Also, it's not clear that using an embedded sccsid string would even work for a wasm/emscripten binary ... Should we just keep not doing any version string in the HOST_BROWSER case then?

Disabled this for now, can be changes again later once it's clear what to do.

@akoeplinger
Copy link
Member

Disabled this for now, can be changes again later once it's clear what to do.

Sounds good to me, thanks!

@akoeplinger akoeplinger merged commit e645b51 into dotnet:main Oct 6, 2021
@uweigand
Copy link
Contributor Author

uweigand commented Oct 6, 2021

Thanks @akoeplinger ! Can still this get included into .NET 6 at this point? This would help the RHEL builds.

@akoeplinger
Copy link
Member

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Oct 6, 2021

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1313127365

@uweigand uweigand deleted the mono-build-compat branch October 6, 2021 18:26
steveisok pushed a commit that referenced this pull request Oct 7, 2021
Backport of #59988

* Link with -z relro -z now to enable full RELRO mode
  Fixes #59904

* Generate and link in version.c to provide version string
  Fixes dotnet/source-build#2484

Co-authored-by: Ulrich Weigand <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Nov 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Build-mono community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[s390x] Missing full RELRO (-z,now) support in a mono-based libcoreclr.so [s390x] Missing commit identifier in libcoreclr.so

3 participants