[llvm] r316064 - Fix the incorrect detection of ICONV_LIBRARY_PATH

Serguei Katkov via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 6 21:31:06 PST 2017


Hi Hans,

the bug we had was due to misconfiguration.

The patch has been posted due to I think on most Linux installations, check for libc is actually always true.
If we believe that iconv is present on libc, it would be good to check it explicitly.

Meanwhile, our issue can workarounded internally, so I'm ok with revert of this patch.

Thank you,
Serguei.

From: Eric Beckmann [mailto:ecbeckmann at google.com]
Sent: Tuesday, November 7, 2017 5:46 AM
To: Hans Wennborg <hans at chromium.org>
Cc: Serguei Katkov <serguei.katkov at azul.com>; llvm-commits <llvm-commits at lists.llvm.org>; Peter Collingbourne <pcc at google.com>
Subject: Re: [llvm] r316064 - Fix the incorrect detection of ICONV_LIBRARY_PATH

The reason this was added in the first place was that certain builds (android?  I think) would break when libxml was present but iconv was not.  Libxml2 uses iconv for some internationalization encoding it does (http://xmlsoft.org/encoding.html).  Even though libxml2 was found on these builds, a compile time error which went something like “fatal error: iconv.h: No such file or directory" would be thrown.  This is perhaps a bug with libxml2 in that it assumes iconv is present in all situations.

I'm not sure why the check was made specific to Linux, perhaps check with Nico, this was done in r315873.

Sorry this was a while ago and this is most of the info I can give.


On Mon, Nov 6, 2017 at 2:18 PM, Hans Wennborg <hans at chromium.org<mailto:hans at chromium.org>> wrote:
I've reverted this in r317517 as it (silently) broke our builds, i.e.
it was no longer setting LLVM_LIBXML2_ENABLED.

Questions inline.

On Tue, Oct 17, 2017 at 11:26 PM, Serguei Katkov via llvm-commits
<llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>> wrote:
> Author: skatkov
> Date: Tue Oct 17 23:26:39 2017
> New Revision: 316064
>
> URL: http://llvm.org/viewvc/llvm-project?rev=316064&view=rev
> Log:
> Fix the incorrect detection of ICONV_LIBRARY_PATH
>
> This is introduced in rL308711.
> Check for c library is incorrect here just because libc will be found always
> and it does not mean that iconv is presented.
>
> Thank to Andrew Krasny for narrowing down the root cause.

What this was the root cause of? I couldn't find any bug, and no
discussion on the code review or mailing list.

> Reviewers: ecbeckmann
> Reviewed By: ecbeckmann
> Subscribers: mgorny, llvm-commits
> Differential Revision: https://reviews.llvm.org/D38875
>
> Modified:
>     llvm/trunk/cmake/config-ix.cmake
>
> Modified: llvm/trunk/cmake/config-ix.cmake
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/cmake/config-ix.cmake?rev=316064&r1=316063&r2=316064&view=diff
> ==============================================================================
> --- llvm/trunk/cmake/config-ix.cmake (original)
> +++ llvm/trunk/cmake/config-ix.cmake Tue Oct 17 23:26:39 2017
> @@ -157,7 +157,7 @@ if( NOT PURE_WINDOWS AND NOT LLVM_USE_SA
>      set(HAVE_TERMINFO 0)
>    endif()
>
> -  find_library(ICONV_LIBRARY_PATH NAMES iconv libiconv libiconv-2 c)
> +  find_library(ICONV_LIBRARY_PATH NAMES iconv libiconv libiconv-2)

On Linux, I believe libc usually provides iconv() which is why the
check was written that way.

>    set(LLVM_LIBXML2_ENABLED 0)
>    set(LIBXML2_FOUND 0)
>    if((LLVM_ENABLE_LIBXML2) AND ((CMAKE_SYSTEM_NAME MATCHES "Linux") AND (ICONV_LIBRARY_PATH) OR APPLE))

But this raises lots of questions.

Why do we need to check for iconv here? Isn't it enough that we find
libxml2; i.e., if we find it, can't we assume that it's usable?

And why is the iconv() check specific to Linux? Is it not needed on
Mac, and what if the user is on FreeBSD or something else -- no
libxml2 for them?

Does anyone know what's going on here?

Thanks,
Hans

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171107/43e91826/attachment.html>


More information about the llvm-commits mailing list