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

AS variable is wrongly set #23102

Open
stsp opened this issue Feb 4, 2025 · 14 comments · May be fixed by #23190 or #23456
Open

AS variable is wrongly set #23102

stsp opened this issue Feb 4, 2025 · 14 comments · May be fixed by #23190 or #23456
Labels

Comments

@stsp
Copy link
Contributor

stsp commented Feb 4, 2025

It seems termux sets AS
variable to aarch64-linux-android-clang
in termux_setup_toolchain_27c.sh script.
But it should set it to:
aarch64-linux-android-clang -c
as otherwise also linker is invoked, leading
to a build failure (in my case).

@Biswa96
Copy link
Member

Biswa96 commented Feb 4, 2025

Could you share how to reproduce the build failure?

@stsp
Copy link
Contributor Author

stsp commented Feb 4, 2025

If only on my own CI:
https://github.com/dosemu2/dosemu2/actions/runs/13136822663/job/36662916337

aarch64-linux-android-clang   -o tmp.o tmp.s
ld.lld: error: undefined symbol: main

is what you see near the end of the log.
Had to override AS variable, but this
is a very bad thing to do as now it may
fail on some other environment...

The run with work-around applied:
https://github.com/dosemu2/dosemu2/actions/runs/13136822663/job/36665587806

Here you can see:

aarch64-linux-android-clang -x assembler -c   -o tmp.o tmp.s

somewhere in the middle of a huge log.

@licy183
Copy link
Member

licy183 commented Feb 9, 2025

I don't know whether setting AS to clang -c will break the build in other cases...

@stsp
Copy link
Contributor Author

stsp commented Feb 9, 2025

I think someone should just
try a full rebuild with that. If
no regressions - commit and
forget. :)

@truboxl truboxl self-assigned this Feb 15, 2025
@truboxl
Copy link
Contributor

truboxl commented Feb 17, 2025

I ran full rebuild of the packages and found no difference in setting AS variable to *-clang -c.
So I believe its safe to make this change.

truboxl#437

@truboxl truboxl linked a pull request Feb 17, 2025 that will close this issue
@stsp
Copy link
Contributor Author

stsp commented Feb 17, 2025

Thank you.
I didn't know it requires
such a huge patch-set.
But I'll be happy to revert
my work-around and test
your work immediately
once applied.

@truboxl
Copy link
Contributor

truboxl commented Feb 22, 2025

My conclusion from the rebuild is that forcing a specific AS will break some builds. The existing set up AS=*clang allows clang to interpret .s and .S differently without forcing -x assembler or -x assembler-with-cpp. Some builds like mogan that use xmake fails to parse AS variable with spaces. So I am not pursuing the change.

Therefore I propose to close this issue as won't fix.

@stsp
Copy link
Contributor Author

stsp commented Feb 22, 2025

-x assembler is optional, so how
about plain -c then?

@truboxl
Copy link
Contributor

truboxl commented Feb 22, 2025

My take is to not to force any command line argument when designate a compiler or assembler for compiling. That should be done by author's Makefile or build system. It has worked for a long time.

I let others to chime in first.

@TomJo2000
Copy link
Member

I let others to chime in first.

Will do.

My take is to not to force any command line argument when designate a compiler or assembler for compiling. That should be done by author's Makefile or build system. It has worked for a long time.

That's been my experience as well.
It's not the job of our build system to prescribe details of the toolchain down to the assembler flags.
If there are relevant differences between toolchains that a project wants to support it's a safe bet that the project will be handling the toolchain specifics itself, and if we do need to make fixes to that handling that is likely to be project specific and will be better served by an override in the build script, or a patch, as applicable.
There's unlikely to be a one size fits all solution in this case.

@stsp
Copy link
Contributor Author

stsp commented Feb 22, 2025

There's unlikely to be a one size fits all solution in this case.

That's exactly what was expected
to be otherwise. I.e. clang -c should
be fully gas-compatible.
So if this one doesn't regress (not
quite clear from this thread), I think
it makes sense to use that one, OR
just not to define AS at all. If AS is
not defined, then the build system
takes care of that.
Not the case when its wrongly defined...

@truboxl
Copy link
Contributor

truboxl commented Feb 22, 2025

I will leave up the rest of the team to decide whether to merge #23190 rebuild report. The gist is there's no difference.

The only other real world example I can find is Gentoo: https://github.com/gentoo/gentoo/blob/master/profiles/features/llvm/make.defaults

The drawback is to manually fix exotic build system for some packages which noted by gentoo/gentoo#27087 (comment)

Still, I found examples of AS=clang and pass -c separately as a argument more than AS=clang -c in the wild, contrary to author's claim of AS=clang is wrong.

@stsp
Copy link
Contributor Author

stsp commented Feb 22, 2025

AS=clang -c in the wild, contrary to author's claim of AS=clang is wrong.

OK, I re-checked if maybe
as reserves -c for compatibility.
It really looks like it does, but
when tracing with gdb, I've found
its actually interpreting -c as
--compress-debug-sections.
So you are not going to notice
the difference most of the time,
but its still not the right thing
to do.
So people who add -c later,
are walking across a thin ice.
--compress-debug-sections
is mostly harmless, but eventually
it will backfire somewhere.

@stsp
Copy link
Contributor Author

stsp commented Feb 23, 2025

Hmm, and actually, how about
simply adding "ASFLAGS=-c"?
If you ever switch AS, you will
also switch ASFLAGS, and the
problem solved?

stsp added a commit to stsp/termux-packages that referenced this issue Feb 24, 2025
@stsp stsp linked a pull request Feb 24, 2025 that will close this issue
stsp added a commit to stsp/termux-packages that referenced this issue Feb 24, 2025
stsp added a commit to stsp/termux-packages that referenced this issue Feb 24, 2025
stsp added a commit to stsp/termux-packages that referenced this issue Feb 24, 2025
stsp added a commit to stsp/termux-packages that referenced this issue Feb 24, 2025
stsp added a commit to stsp/termux-packages that referenced this issue Feb 24, 2025
stsp added a commit to stsp/termux-packages that referenced this issue Feb 24, 2025
stsp added a commit to stsp/termux-packages that referenced this issue Feb 24, 2025
stsp added a commit to stsp/termux-packages that referenced this issue Feb 24, 2025
stsp added a commit to stsp/termux-packages that referenced this issue Feb 24, 2025
stsp added a commit to stsp/termux-packages that referenced this issue Feb 24, 2025
stsp added a commit to stsp/termux-packages that referenced this issue Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants