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

t1utils/1.40 miscompiles without -D_GNU_SOURCE #8

Closed
nthykier opened this issue Jul 26, 2017 · 9 comments
Closed

t1utils/1.40 miscompiles without -D_GNU_SOURCE #8

nthykier opened this issue Jul 26, 2017 · 9 comments

Comments

@nthykier
Copy link
Contributor

Hi,

I received a bug that my 1.40 upload of t1utils to Debian was broken because "memmem" was implicitly declared. This causes the compiler to assume it returns "int" instead of a pointer, which unsurprisingly leads to very broken results.

I see three things we can do to improve this:

  1. The build system/code should ensure the function is declared before use. -D_GNU_SOURCE works for my case, but may not be portable to all target platforms you want to support (at least, I presume that is the case, since you wrote a compat version of memmem()).

  2. I think it would be prudent make the implicit declaration a hard error, given how unlikely it is that we ever want that. I think the gcc syntax for that is -Werror=implicit-function-declaration. At least, I intend to enable that in the packaging (but I would probably check it too late to catch it in your releases).

  3. Add a regression test suite to see that basic functionally works.

(Original report in Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=869756)

FYI, I got another bug coming - hopefully later today if time permits.

@nthykier
Copy link
Contributor Author

Indeed, -Werror=implicit-function-declaration is the correct syntax for gcc.

@kohler
Copy link
Owner

kohler commented Aug 16, 2017

3f1ddda has my preferred fix for this issue

@nthykier
Copy link
Contributor Author

Sorry for the delay. :)

I have tried the patch and it does seem to fix the miscompile. I have tested that the build succeeds with -Werror=implicit-function-declaration (and fails without your changes). :)

For the Debian package, I will keep the -D_GNU_SOURCE for now as I assume the platform provided version of memmem is optimized better.

Thanks for fixing the issue. :)

@lucaswerkmeister
Copy link

Sorry to bother you, but could you perhaps publish a release version with this fix? Arch, Manjaro and Gentoo users are currently unable to build LilyPond because t1asm segfaults when a 64-bit pointer is truncated to 32-bit int due to the missing memmem declaration. (I’m not sure if it affects all Arch/Manjaro/Gentoo users on 64-bit platforms, but you can find several reports in this thread.)

@fedelibre
Copy link

For the records, I've filed a bug report to Fedora tracker to include the patch in Fedora.

@fedelibre
Copy link

I understand that only one commit, which was not included in 1.41 git tag and source tarball for a mistake, does not justify a new release. What about pointing the tag to that commit and upload a new tarball here?

In the meanwhile I'll propose a patch to include that commit in Fedora package.
But fixing the tarball would be more correct IMO and solve the problem for any distro.

@hanwen
Copy link

hanwen commented Feb 13, 2020

@kohler - gentle ping?

@dvzrv
Copy link

dvzrv commented Oct 26, 2020

@kohler can you please tag a new version?
Unpatched t1utils breaks the build for lilypond on Arch Linux (https://bugs.archlinux.org/task/57999).

Thanks!

@kohler
Copy link
Owner

kohler commented Oct 27, 2020

Done

@kohler kohler closed this as completed Oct 27, 2020
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

No branches or pull requests

6 participants