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

Kojoley commented 4 months ago

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 commented 4 months ago

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

pdimov commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

pdimov commented 4 months ago

Is this approach going to work for cross compilation scenarios?

Kojoley commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

grafikrobot commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

@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 commented 4 months ago

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.

bfgroup / b2