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

gcc/clang: -m* flags when architecture is not explicitly set #379

Conversation

Kojoley
Copy link
Contributor

@Kojoley Kojoley commented May 1, 2024

Proposed changes

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.

Types of changes

What types of changes does your code introduce?

Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Documentation content changes
  • Other (please describe):

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I searched the discussions
  • I searched the closed and open issues
  • I read the contribution guidelines
  • I added myself to the copyright attributions for significant changes
  • I checked that tests pass locally with my changes
  • I added tests that prove my fix is effective or that my feature works
  • I added necessary documentation (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@grisumbras
Copy link
Contributor

The PR appears to have exposed CI build environments not having been properly set up for 32 bit builds.

@Kojoley Kojoley force-pushed the feature/fix-address-model-when-arch-is-empty branch from d4c347a to 42fae3a Compare May 1, 2024 09:38
@pdimov
Copy link
Contributor

pdimov commented May 1, 2024

Was that the feature where address-model=32,64 would silently succeed and build everything twice for e.g. s390x that doesn't have 32 bit?

@Kojoley
Copy link
Contributor Author

Kojoley commented May 1, 2024

Was that the feature where address-model=32,64 would silently succeed and build everything twice for e.g. s390x that doesn't have 32 bit?

Happy accident, as far as I can tell, like anytime when b2 silently ignores your inputs.

There is multiple issues with s390x:

  1. address-model on s390x is completely ignored.
  2. s390x is like amd64/x86_64, for consistency with other architectures it should've been s390, but
  3. IIUC GCC doesn't support generating code for s390, but it can generate code for s390 abi, something close to -mx32 on x86_64.
  4. Clang/LLVM-based compiler for s390x is paywalled.

So it seems like it would not hurt to set -m31 on architecture=s390x address-model=32.

There is also the same issue for riscv, but I don't know how to resolve it.

@Kojoley
Copy link
Contributor Author

Kojoley commented May 1, 2024

@grafikrobot is there a really need to ensure warnings-free build with architecture=32,64 on for every GCC and Clang out there?

@pdimov
Copy link
Contributor

pdimov commented May 1, 2024

Is this approach going to work for cross compilation scenarios?

@Kojoley
Copy link
Contributor Author

Kojoley commented May 1, 2024

Is this approach going to work for cross compilation scenarios?

Could you please expand on your question? You mean using gcc : : gcc-ppc64le-linux-gnu ; b2 toolset=gcc address-model=32 or?

@Kojoley
Copy link
Contributor Author

Kojoley commented May 1, 2024

I can't come up with a scenario where this approach will fail, even using clang : : clang --target=ppc64le-linux-gnu ; b2 toolset=clang address-model=32 or using clang : : clang --target=ppc64le-linux-gnu -m32 ; b2 toolset=clang address-model=64 will do what you logically expect it should do (first will target powerpcle and second will target ppc64le).

@pdimov
Copy link
Contributor

pdimov commented May 1, 2024

I suppose that if you put the --target option in user-config it will be properly reflected by -dumpmachine, yes.

@Kojoley Kojoley force-pushed the feature/fix-address-model-when-arch-is-empty branch from 42fae3a to 9ee19b1 Compare May 1, 2024 20:24
@grafikrobot
Copy link
Member

@grafikrobot is there a really need to ensure warnings-free build with architecture=32,64 on for every GCC and Clang out there?

It's caught some errors in the past. But I'm willing to change it. If you can offer some suggestions.

@grafikrobot
Copy link
Member

@grafikrobot is there a really need to ensure warnings-free build with architecture=32,64 on for every GCC and Clang out there?

It's caught some errors in the past. But I'm willing to change it. If you can offer some suggestions.

PS. The failures here for the no-warnings step are not because of warnings though. They appear to be real errors on first glance.

@Kojoley
Copy link
Contributor Author

Kojoley commented May 4, 2024

@grafikrobot is there a really need to ensure warnings-free build with architecture=32,64 on for every GCC and Clang out there?

It's caught some errors in the past. But I'm willing to change it. If you can offer some suggestions.

I mean... architecture=32 did nothing, so it would not catch anything.

What makes sense to me is to disable warning for bootstrap, and test for warnings only on recent compilers.

I also think there is no real users that install clang from llvm repository or gcc from ppa on ubuntu, especially on discontinued versions. I would only test default compiler on discontinued platforms and not bother with third-party repositories.

@grafikrobot is there a really need to ensure warnings-free build with architecture=32,64 on for every GCC and Clang out there?

It's caught some errors in the past. But I'm willing to change it. If you can offer some suggestions.

PS. The failures here for the no-warnings step are not because of warnings though. They appear to be real errors on first glance.

I stopped trying to understand what is needed to install for multilib clang on those failed jobs, but gcc<6 false-positive is there:

src/engine/jam.cpp: In function 'int guarded_main(int, char**)':
src/engine/jam.cpp:410:30: error: array subscript is below array bounds [-Werror=array-bounds]
             globs.debug[ i-- ] = 1;
                              ^
cc1plus: all warnings being treated as errors

@Kojoley Kojoley force-pushed the feature/fix-address-model-when-arch-is-empty branch 6 times, most recently from d1e8c85 to e93f39c Compare May 5, 2024 04:11
@Kojoley
Copy link
Contributor Author

Kojoley commented May 5, 2024

My experiments show that Clang support -m32/-m64 universally (and doesn't support -m31 for s390*), so I dropped GCC logic in it and just made Clang put -m* flag solely on value.

@grafikrobot the change to azure scripts mostly no-op (dropping address-model=32,64 that were doing nothing before the patch). I've made Linux_Latest to really perform address-model=32,64.

azure-pipelines.yml Outdated Show resolved Hide resolved
@Kojoley Kojoley force-pushed the feature/fix-address-model-when-arch-is-empty branch from e93f39c to d0be376 Compare May 9, 2024 12:54
GCC is picky, -m32/-m64 flags are available only on a subset of targets. Clang supports -m32/-m64 universally.
@Kojoley Kojoley force-pushed the feature/fix-address-model-when-arch-is-empty branch from d0be376 to 3c82315 Compare May 9, 2024 13:02
@grafikrobot grafikrobot merged commit 006b1ec into bfgroup:main May 9, 2024
142 checks passed
@Kojoley Kojoley deleted the feature/fix-address-model-when-arch-is-empty branch May 9, 2024 19:08
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.

4 participants