Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FreeBSD build #19623

Closed
wants to merge 2 commits into from
Closed

Fix FreeBSD build #19623

wants to merge 2 commits into from

Conversation

@devnexen
Copy link

devnexen commented Aug 23, 2018

No description provided.

@dnfclas

This comment has been minimized.

Copy link

dnfclas commented Aug 23, 2018

CLA assistant check
All CLA requirements met.

Copy link
Member

janvorli left a comment

Thank you for fixing the build, I have a couple of comments.

@@ -98,7 +98,11 @@ else (WIN32)
enable_language(ASM)

# Ensure that awk is present
find_program(AWK awk)
if (CMAKE_SYSTEM_NAME STREQUAL "FreeBSD")

This comment has been minimized.

Copy link
@janvorli

janvorli Aug 23, 2018

Member

This change is not needed, there is awk on FreeBSD too. If you want to enable gawk as an option for people that don't want to have awk installed, then please add a second find_program just without if. The find_program looks for the target program only when the AWK is not set yet or it is set to AWK-NOTFOUND.

This comment has been minimized.

Copy link
@devnexen

devnexen Aug 23, 2018

Author

In fact it was to avoid the risk of different output (in future) between bsd awk and gnu awk but I can revert it no problems.

@@ -7,6 +7,9 @@
#include <cassert>
#include <memory>
#include <pthread.h>
#ifdef __FreeBSD__

This comment has been minimized.

Copy link
@janvorli

janvorli Aug 23, 2018

Member

I can see that we differ in the way to get the thread id here and in the https://github.com/dotnet/coreclr/blob/master/src/pal/src/include/pal/thread.hpp#L838-L844. Could you please unify it? If both ways return the same values, then I'd use the pthread_getthreadid_np at both places. If not, then please let's use the thr_self as it is verified to provide the same identifier as LLDB, which we need in the LLDB plugin.

Copy link
Member

janvorli left a comment

LGTM, thank you!

@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Oct 25, 2018

cc: @wfurt

devnexen added 2 commits Aug 23, 2018
Harmonizing use of thread id getter.
@devnexen devnexen force-pushed the devnexen:freebsd_build_fix branch from 02f73e1 to 576709c Feb 25, 2019
@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Mar 15, 2019

@wfurt, should this be merged? Closed?

@wfurt

This comment has been minimized.

Copy link
Member

wfurt commented Mar 15, 2019

Current master should build on FreeBSD. However pthread_getthreadid_np() seems cleaner to me. I'm not sure if there is any practical impact. We also had issues with threading on 12.0. I don't know if this can have any impact. Do you have any ideas @devnexen ???

@devnexen

This comment has been minimized.

Copy link
Author

devnexen commented Mar 15, 2019

API-wise I would also prefer pthread_getthreadid_np but in this case the outcome should not change much regarding the use case here.

@jkotas jkotas added the area-Build label Nov 2, 2019
@maryamariyan

This comment has been minimized.

Copy link
Member

maryamariyan commented Nov 6, 2019

Thank you for your contribution. As announced in #27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:

  1. In your coreclr repository clone, create patch by running git format-patch origin
  2. In your runtime repository clone, apply the patch by running git apply --directory src/coreclr <path to the patch created in step 1>
@maryamariyan

This comment has been minimized.

Copy link
Member

maryamariyan commented Dec 2, 2019

Thank you for your contribution. As announced in #27549 the dotnet/runtime repository will be used going forward for changes to this code base. Closing this PR as no more changes will be accepted into master for this repository. If you’d like to continue working on this change please move it to dotnet/runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.