-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fix illumos compile errors in coreclr/tools/superpmi/mcs/verbmerge.cpp #117464
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
Conversation
@dotnet/jit-contrib Could you please review? |
@gwr, we are not expected to keep syncing our PRs with main branch in this busy, ever-changing, repo. The maintainer can decide to merge main into the PR branch (just once) depending on the context / staleness of last CI run etc. Secondly, Pri0 tests are still failing. You can do a minimal change like done here #34263, that didn't break existing platforms and made things just work on illumos. If you want to stick with your version, then as I suggested earlier, do yourself a favor and move your code under Finally, superpmi instrumentation stuff is not important . You will very likely never actually need to run the instrumented JIT on illumos (as it is never been ran on freebsd since 2015..) so all this back and forth comments is basically just to quiet a compilation error of an unused component.. Is it really worth the hassle? |
I think the Pri0 tests look OK now. I don't see a simpler or more narrow way to fix this. The only easy place to add the stat call is in the readdir loop, which needs the (minor) change to how filter is called. |
Minor cleanup, then rebase and squash so it's easier to deal with locally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lGTM, thank you!
/azp run runtime-coreclr superpmi-collect-test |
Azure Pipelines successfully started running 1 pipeline(s). |
Anything else I should do with this? Thanks. (bump) |
Sorry, this fell off my radar. I will rerun the CI pipeline. |
/azp run runtime-coreclr superpmi-collect-test |
Azure Pipelines successfully started running 1 pipeline(s). |
Looks like outerloop is broken but is being worked on (#118241 (comment)), so will have to wait for that first to run the pipeline. |
/azp run runtime-coreclr superpmi-collect-test |
Azure Pipelines successfully started running 1 pipeline(s). |
Two of the tests continue to show as "running" a week later. Is that expected? |
Hmm no, looks like another infra oddity... |
/azp run runtime-coreclr superpmi-collect-test |
Azure Pipelines successfully started running 1 pipeline(s). |
Looks like a few tests are again not completing. (bump) |
/runtime/src/coreclr/tools/superpmi/mcs/verbmerge.cpp: In static member function 'static bool verbMerge::DirectoryFilterDirectories(FilterArgType*)': /runtime/src/coreclr/tools/superpmi/mcs/verbmerge.cpp:188:19: error: 'verbMerge::FilterArgType' {aka 'struct dirent'} has no member named 'd_type' 188 | if (findData->d_type == DT_DIR) | ^~~~~~ For the fix, do similarly as: src/native/corehost/hostmisc/pal.unix.cpp Let FilterArgType use struct FindData to simplify filters. Handle the possibility of finding d_type == DT_UNKOWN Add HAVE_DIRENT_D_TYPE introspection Co-authored-by: Adeel Mujahid <[email protected]>
Rebased onto main as of Aug 30. No other changes. |
Thanks! I think the timeouts we saw in superpmi-collect-test were unrelated. Let's see if we run into any issues. |
When cross-compiling for illumos, which has no "d_type" field in struct dirent:
and similar a few other places in this file
For the fix, do similarly as: src/native/corehost/hostmisc/pal.unix.cpp
Let FilterArgType use struct FindData to simplify filters.
Handle the possibility of finding d_type == DT_UNKOWN
This part is from @am11 (Thanks!)
Add HAVE_DIRENT_D_TYPE introspection