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

Ensure we take compiler-provided declaration of posix_memalign #4609

Merged
merged 5 commits into from
Apr 18, 2024

Conversation

asl
Copy link
Contributor

@asl asl commented Apr 11, 2024

Some systems (e.g. those with GNU libc) declare posix_memalign with an exception specifier. Compilers (clang including) might have special handling of this to allow posix_memalign redeclaration with / without exception specifier. As we override posix_memalign for GC purposes we really need to ensure the proper include order to workaround this weirdness.

@asl asl added core Topics concerning the core segments of the compiler (frontend, midend, parser) run-sanitizer Use this tag to run a Clang+Sanitzers CI run. labels Apr 11, 2024
@fruffy
Copy link
Collaborator

fruffy commented Apr 11, 2024

Thanks, we can merge this instead of #4595. They are doing similar things.

@asl
Copy link
Contributor Author

asl commented Apr 11, 2024

@fruffy Yes, I am checking different approaches and how the problem does manifest.

Copy link
Collaborator

@fruffy fruffy left a comment

Choose a reason for hiding this comment

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

Conditionally approving assuming COMPILE_WITH_CLANG: ON is removed.

@asl
Copy link
Contributor Author

asl commented Apr 17, 2024

@fruffy How about adding builder that would build p4c using clang on Ubuntu?

@vlstill
Copy link
Contributor

vlstill commented Apr 17, 2024

@fruffy How about adding builder that would build p4c using clang on Ubuntu?

Considering the discussion I had with @fruffy regarding the static builds, this should be probably part of nightly builds not PR suite (the risk of breakage would be relatively small I guess, especially breakage just under clang and not on macOS). I think having a clang build in CI would be useful.

@fruffy
Copy link
Collaborator

fruffy commented Apr 17, 2024

We do already have a Clang nightly build, it is the sanitizer one. It just does not use unity builds.

https://github.com/p4lang/p4c/blob/main/.github/workflows/ci-ubuntu-20-sanitizer-nightly.yml#L21

@asl
Copy link
Contributor Author

asl commented Apr 18, 2024

We do already have a Clang nightly build, it is the sanitizer one. It just does not use unity builds.

Ah, great. How does it do notifications?

@fruffy
Copy link
Collaborator

fruffy commented Apr 18, 2024

We do already have a Clang nightly build, it is the sanitizer one. It just does not use unity builds.

Ah, great. How does it do notifications?

Notifications are kind of strange here: https://docs.github.com/en/actions/monitoring-and-troubleshooting-workflows/notifications-for-workflow-runs

The last user that modified the workflow (me) gets notified of failures.

lib/gc.cpp Outdated Show resolved Hide resolved
@asl
Copy link
Contributor Author

asl commented Apr 18, 2024

@fruffy Ok, I think this is the least bad approach, still not perfect, but we'd have to live with this as-is. Merging when ready.

@fruffy fruffy changed the title [Do not merge] Check posix_memalign prototype Check posix_memalign prototype Apr 18, 2024
@fruffy fruffy disabled auto-merge April 18, 2024 13:41
@fruffy fruffy enabled auto-merge April 18, 2024 13:42
@asl asl changed the title Check posix_memalign prototype Ensure we take compiler-provided declaration of posix_memalign Apr 18, 2024
@asl
Copy link
Contributor Author

asl commented Apr 18, 2024

@fruffy Looks like ptf-ebpf fails due to some network issue?

@fruffy
Copy link
Collaborator

fruffy commented Apr 18, 2024

@fruffy Looks like ptf-ebpf fails due to some network issue?

Looks like a spurious failure, I restarted the test. It's optional so it should not block merging anyway.

@asl
Copy link
Contributor Author

asl commented Apr 18, 2024

Looks like a spurious failure, I restarted the test. It's optional so it should not block merging anyway.

I already restarted it 3 or 4 times...

@fruffy
Copy link
Collaborator

fruffy commented Apr 18, 2024

Looks like a spurious failure, I restarted the test. It's optional so it should not block merging anyway.

I already restarted it 3 or 4 times...

Ah just saw. Hmm, there is nothing we can really do about it for now. I would ignore it. If the issue persists we can start to look into it. This particular test is not super important.

@fruffy fruffy added this pull request to the merge queue Apr 18, 2024
Merged via the queue into p4lang:main with commit c7d688a Apr 18, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser) run-sanitizer Use this tag to run a Clang+Sanitzers CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants