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

Bug 5160: Compile fails with -flto=auto [-Walloc-size-larger-than=] #1118

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sergiodj
Copy link
Contributor

@sergiodj sergiodj commented Aug 10, 2022

When building with LTO support (-flto=auto -ffat-lto-objects)
the following errors occur:

store/Disks.cc:690: error: argument 1 value 18446744073709551615
exceeds maximum object size 9223372036854775807
[-Werror=alloc-size-larger-than=]
const auto tmp = new SwapDir::Pointer[swap->n_allocated];

pconn.cc:43:53: error: argument 1 value 18446744073709551615
exceeds maximum object size 9223372036854775807
[-Werror=alloc-size-larger-than=]
theList_ = new Comm::ConnectionPointer[capacity_];

Arguably, this is likely a compiler bug and not squid's fault. Either
way, it's worth having more type safety in this code.

GCC 12 emits an error when compiling squid due to
-Werror=alloc-size-larger-than.  The error looks like this:

store/Disks.cc:690:64: error: argument 1 value '18446744073709551615' exceeds maximum object size 9223372036854775807 [-Werror=alloc-size-larger-than=]
  690 |         const auto tmp = new SwapDir::Pointer[swap->n_allocated];
      |                                                                ^

Arguably, this is likely a compiler bug and not squid's fault.  Either
way, I believe it's worth having more type safety in this code.

Signed-off-by: Sergio Durigan Junior <[email protected]>
@squid-prbot
Copy link
Collaborator

Can one of the admins verify this patch?

@squid-anubis squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Aug 10, 2022
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Interesting error! Thank you for trying to improve Squid code while working around alleged GCC bugs.

src/SquidConfig.h Outdated Show resolved Hide resolved
src/pconn.h Outdated Show resolved Hide resolved
@rousskov rousskov changed the title Fix FTBFS due to -Werror=alloc-size-larger-than on GCC 12 Fix GCC v12 build [-Walloc-size-larger-than] Aug 10, 2022
@rousskov rousskov changed the title Fix GCC v12 build [-Walloc-size-larger-than] Fix GCC v12 build [-Walloc-size-larger-than=] Aug 10, 2022
@squid-anubis squid-anubis removed the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Aug 10, 2022
@rousskov rousskov marked this pull request as draft August 10, 2022 20:14
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Feb 2, 2023
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Feb 15, 2023
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Mar 29, 2023
@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Jan 9, 2024
@kinkie kinkie added the M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels label Mar 15, 2024
@juju4
Copy link

juju4 commented Aug 17, 2024

is this still under work/consideration?

I am getting this error when compiling squid for tls inspection on ubuntu-24.04 (debian package rebuild) as per https://github.com/juju4/ansible-squid/actions/runs/10429779309/job/28887616336#step:8:1
No issue on 22.04.

Thanks

@rousskov
Copy link
Contributor

is this still under work/consideration?

We still want to improve warning-related code, but it looks like nobody is working on that right now. If you can post an alternative PR (that takes my review concerns into consideration), please do so.

@yadij
Copy link
Contributor

yadij commented Nov 1, 2024

FYI; Please check whether you are building with LTO support enabled by default. This n_allocated size issue shows up reliably with LTO, you may be able to build fine with LTO disabled.

@juju4
Copy link

juju4 commented Nov 2, 2024

FYI; Please check whether you are building with LTO support enabled by default. This n_allocated size issue shows up reliably with LTO, you may be able to build fine with LTO disabled.

On https://github.com/juju4/ansible-squid/actions/runs/11621187815/job/32364459556, I can see compilation arguments "-flto=auto -ffat-lto-objects" but no other clear logs match. Outside of enabling openssl, the rest of the options are part of default debian package.

@yadij yadij changed the title Fix GCC v12 build [-Walloc-size-larger-than=] Fix LTO build [-Walloc-size-larger-than=] Nov 3, 2024
@yadij yadij changed the title Fix LTO build [-Walloc-size-larger-than=] Bug 5160: Compile fails with -flto=auto [-Walloc-size-larger-than=] Nov 3, 2024
@yadij
Copy link
Contributor

yadij commented Nov 3, 2024

arguments "-flto=auto -ffat-lto-objects"

Okay, that is the known LTO bug. Thank you for working on it. I will adjust the title appropriately.

@juju4
Copy link

juju4 commented Nov 3, 2024

What's the impact of disabling LTO? how to do it? no such option in ./configure --help and adding --without-lto does not change anything and result still in non compilable sources.

@yadij yadij requested a review from rousskov November 4, 2024 06:32
@yadij yadij marked this pull request as ready for review November 4, 2024 06:32
@yadij
Copy link
Contributor

yadij commented Nov 4, 2024

OK to test

@yadij yadij added S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box and removed M-ignored-by-merge-bots https://github.com/measurement-factory/anubis/blob/master/README.md#pull-request-labels labels Nov 4, 2024
@squid-anubis squid-anubis added the M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels label Nov 4, 2024
src/pconn.h Show resolved Hide resolved
@rousskov
Copy link
Contributor

rousskov commented Nov 4, 2024

@juju4, I will try to answer your questions, but LTO is not my area of expertise:

What's the impact of disabling LTO?

The exact full impact of building with or without LTO is unknown (to me). As earlier Bug 5160 work indicates, LTO may trigger1 runtime problems without breaking Squid compilation. LTO is also suspected of breaking compilation (this PR and yours). I do not know why.

how to do it?

I do not know and failed to find a good answer by glancing through -flto=n documentation in GCC manual page. I recommend researching this question to get a definitive answer, setting/adjusting CFLAGS and CXXFLAGS environmental variables accordingly, and verifying how the actual compiler flags look when Squid is compiled and linked. Ideally, you want to demonstrate that explicitly enabling LTO breaks build while explicitly disabling LTO avoids the problem.

no such option in ./configure --help and adding --without-lto does not change anything

Correct: Squid is currently unaware of LTO. Compiler is free to enable it (where LTO does not contradict other compiler settings). To control LTO, one should manipulate compiler flags directly. It may be possible to disable LTO as a side effect of using ./configure ... --disable-optimizations as well, but I have not tested that theory and do not recommend using that crude method except as the last resort.

Footnotes

  1. To be more precise, in the original bug 5160 use case, LTO reveals/exposes hidden Squid bugs that affect execution of test cases.

@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) and removed S-waiting-for-reviewer ready for review: Set this when requesting a (re)review using GitHub PR Reviewers box labels Nov 4, 2024
@juju4
Copy link

juju4 commented Nov 4, 2024

Confirming that building without LTO is successful on Ubuntu 24.04 as per https://github.com/juju4/ansible-squid/actions/runs/11669115650/job/32490348166#step:8:532 but fails for 22.04 or earlier
I did as gentoo prescribed on https://wiki.gentoo.org/wiki/LTO#Disable_LTO_per_Package with CFLAGS and others.

On LTO, mostly from gcc docs and stackoverflow and matching the "introduced subtle bugs"
https://gcc.gnu.org/onlinedocs/gccint/LTO-Overview.html
https://stackoverflow.com/questions/23736507/is-there-a-reason-why-not-to-use-link-time-optimization-lto

@rousskov
Copy link
Contributor

rousskov commented Nov 4, 2024

Confirming that building without LTO is successful on Ubuntu 24.04 as per https://github.com/juju4/ansible-squid/actions/runs/11669115650/job/32490348166#step:8:532

FWIW, that build log shows -flto=auto in LDFLAGS. I know that Gentoo instructions you found do not include LDFLAGS, and I do not know whether LDFLAGS alone are enough to enable some LTO aspects despite -fno-lto in CXXFLAGS, but it does not look like a clean "without LTO" build to me.

@rousskov
Copy link
Contributor

rousskov commented Nov 4, 2024

I adjusted PR description to fix formatting, to add the second known error to the "errors" list, and to restore the original "likely a compiler bug" speculation.

@squid-anubis squid-anubis added M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels and removed M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels M-failed-other https://github.com/measurement-factory/anubis#pull-request-labels labels Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-failed-description https://github.com/measurement-factory/anubis#pull-request-labels S-waiting-for-author author action is expected (and usually required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants