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

In the old version of glibc and O2 is enabled, replacing the strncmp argument with a macro will cause the compilation to fail #29291

Closed
hardboydu opened this issue Jun 12, 2024 · 14 comments · Fixed by #29319
Assignees
Labels
bug issues reporting wrong behavior build building and installing Neovim using the provided scripts has:bisected issue has been tracked to a specific commit

Comments

@hardboydu
Copy link

hardboydu commented Jun 12, 2024

Problem

neovim-nightly/src/nvim/autocmd.c:1248:43: error: macro "strncmp" requires 3 arguments, but only 2 given
 1248 |   if (strncmp(*argp, S_LEN("<nomodeline>")) == 0) {
      |                                           ^
In file included from /usr/include/string.h:633,
                 from neovim-nightly/src/nvim/autocmd.c:8:
/usr/include/bits/string2.h:919: note: macro "strncmp" defined here
  919 | # define strncmp(s1, s2, n)                                                   \
      |
neovim-nightly/src/nvim/autocmd.c:2362:54: error: macro "strncmp" requires 3 arguments, but only 2 given
 2362 |   return patlen >= 8 && strncmp(pat, S_LEN("<buffer")) == 0 && (pat)[patlen - 1] == '>';
      |                                                      ^
/usr/include/bits/string2.h:919: note: macro "strncmp" defined here
  919 | # define strncmp(s1, s2, n)                                                   \
      |
make[2]: *** [src/nvim/auto/autocmd.c.generated.h] Error 1

Steps to reproduce

cmake ../neovim-nightly \
		-DCMAKE_INSTALL_PREFIX=/opt/neovim \
		-DCMAKE_BUILD_TYPE=Release

Expected behavior

Compilation succeeded

Neovim version (nvim -v)

commit 66a1e02

Vim (not Nvim) behaves the same?

N/A

Operating system/version

CentOS 7.9

Terminal name/version

windows terminal

$TERM environment variable

xterm-256color

Installation

build from repo

@hardboydu hardboydu added the bug issues reporting wrong behavior label Jun 12, 2024
@zeertzjq zeertzjq added the build building and installing Neovim using the provided scripts label Jun 12, 2024
@dundargoc

This comment has been minimized.

@dundargoc dundargoc added the needs:response waiting for reply from the author label Jun 12, 2024
@hardboydu

This comment has been minimized.

@github-actions github-actions bot removed the needs:response waiting for reply from the author label Jun 12, 2024
@dundargoc

This comment was marked as off-topic.

@dundargoc dundargoc self-assigned this Jun 12, 2024
@hardboydu

This comment was marked as off-topic.

@clason

This comment was marked as off-topic.

@dundargoc

This comment has been minimized.

@dundargoc dundargoc added the needs:response waiting for reply from the author label Jun 13, 2024
@moonstruckmoth
Copy link
Sponsor

I'm facing the same issue and I've tracked down the commit from where it started failing to compile.

The commit is as below

commit c37695a (HEAD)
Author: James 89495599+IAKOBVS@users.noreply.github.com
Date: Tue Jun 11 22:40:24 2024 +0700

refactor: use S_LEN(s) instead of s, n (#29219)

@clason clason added has:bisected issue has been tracked to a specific commit and removed needs:response waiting for reply from the author labels Jun 13, 2024
@dundargoc
Copy link
Member

@IAKOBVS

@IAKOBVS
Copy link
Contributor

IAKOBVS commented Jun 13, 2024

@IAKOBVS

S_LEN macro doesn't work when passed as an argument to another macro. That's why I didn't use it for the STRNICMP macros, only functions.

@clason
Copy link
Member

clason commented Jun 13, 2024

Yes, obviously. Can you fix this so it works on all supported platforms, or should we revert that commit?

@IAKOBVS
Copy link
Contributor

IAKOBVS commented Jun 13, 2024

Yes, obviously. Can you fix this so it works on all supported platforms, or should we revert that commit?

Just revert it.

@gpanders
Copy link
Member

Is this something we can test for in CI? Otherwise how we will know if any use of S_LEN is safe? (Unless we make a rule: "never use S_LEN in stdlib functions because those could secretly be macros").

@clason
Copy link
Member

clason commented Jun 13, 2024

Not without containers. (But I believe this sort of "macro wrappers for gcc" in glibc can be researched; It will take some effort, though.)

@dundargoc
Copy link
Member

Is this something we can test for in CI? Otherwise how we will know if any use of S_LEN is safe? (Unless we make a rule: "never use S_LEN in stdlib functions because those could secretly be macros").

I don't see how we could. Even if we start testing centos specifically it won't account for other distros. We could start testing a slew of popular distros (centos, debian, fedora, arch, manjaro, etc etc) but that is going to be expensive CI-wise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug issues reporting wrong behavior build building and installing Neovim using the provided scripts has:bisected issue has been tracked to a specific commit
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants