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

Build in debug mode on ClangUbsanLinuxBuild #340

Closed
wants to merge 1 commit into from
Closed

Build in debug mode on ClangUbsanLinuxBuild #340

wants to merge 1 commit into from

Conversation

kumaraditya303
Copy link

@kumaraditya303 kumaraditya303 commented Sep 10, 2022

@pablogsal
Copy link
Member

Hummm I have two reservations with this:

  • We should only merge this if the tests are passing. We cannot have a stable buildbot suddenly turning red.
  • Is unclear if this configuration will behave as a superset of release mode. Indeed, release mode and debug mode are technically two different builds so is still possible we will be missing things. It is much safer to test in the mode we intent to release to users so if we want this we should do it in addition to the release mode testing.

@kumaraditya303
Copy link
Author

We should only merge this if the tests are passing. We cannot have a stable buildbot suddenly turning red.

We can test the fixes if this is merged. Having a buildbot failing for a day or two is not that bad. I will be looking into the failures myself.

Is unclear if this configuration will behave as a superset of release mode. Indeed, release mode and debug mode are technically two different builds so is still possible we will be missing things. It is much safer to test in the mode we intent to release to users so if we want this we should do it in addition to the release mode testing.

This will be superset of release mode. Currently the code is relying on compiler optimizations to avoid UB whereas the code should not relying on any compiler behavior. IMO CPython should not be relying on any compiler optimizations' and the C code should be strictly standard conforming and avoid any UB in any case release, debug or whatever.

@pablogsal
Copy link
Member

We can test the fixes if this is merged. Having a buildbot failing for a day or two is not that bad. I will be looking into the failures myself.

Unfortunately it is that bad. By policy we even revert all changes that break buildbots in 24h hours no matter if they unveiled existing problems or if they are unrelated. We take this very seriously.

The way we have done this in the past if fix everything and then change the buildbots.

@pablogsal
Copy link
Member

This will be superset of release mode.

How do you know? I think you are assuming that debug mode is release mode + things on top but that's not always true.

Currently the code is relying on compiler optimizations to avoid UB

That's likely true but is still a guess. We don't have proof of that and nobody has yet spent time to understand how that's manifesting

whereas the code should not relying on any compiler behavior.

I agree but unfortunately it is happening currently.

IMO CPython should not be relying on any compiler optimizations' and the C code should be strictly standard conforming and avoid any UB in any case release, debug or whatever.

Agreed, but unfortunately that's not what happens today. There is code that only runs on debug mode and some code that only runs on release mode (at least in the past we had that to not trip the refleak mode).

@kumaraditya303
Copy link
Author

kumaraditya303 commented Sep 10, 2022

I will investigate the failures and report back.

@matthiasgoergens
Copy link

It is much safer to test in the mode we intent to release to users so if we want this we should do it in addition to the release mode testing.

In general, I agree: if compute resources permit, we should be adding build bot runs, instead of replacing them.

@kumaraditya303
Copy link
Author

We don't need this anymore as the bugs have been fixed, and yes it was compiler optimization thing which is only enabled in debug builds causing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants