Re: [Kaldi-developers] Windows patches to src/ (mostly)

[Daniel P. <dp...@gm...>]

Removing kaldi-developers via bcc.
Obviously it's very confusing.  However, I notice in your patch to
kaldi-types.h, if using MSVC you are including basestd.h instead of
stdint.h, while OpenFst uses stdint.h.  If we can ensure that the
types resolve to the same thing, things should work regardless of the
compiler bug.  I think the easiest fix might be to just use stdint.h,
the same as OpenFst, when using MSVC- if it is available.
Dan


On Tue, May 19, 2015 at 5:20 PM, Kirill Katsnelson
<kir...@sm...> wrote:
> No dice. In fact, I have patched openfstwin (1.3.1 currently), and my fts/types.h looks like this, verbatim:
>
> #include <cstdlib>       // for ssize_t
> #include <stdint.h>      // *int*_t
>
> #include <fst/compat.h>  // for DISALLOW_COPY_AND_ASSIGN
>
> #ifndef FST_LIB_TYPES_H__
> #define FST_LIB_TYPES_H__
>
> typedef int8_t int8;
> typedef int16_t int16;
> typedef int32_t int32;
> typedef int64_t int64;
>
> typedef uint8_t uint8;
> typedef uint16_t uint16;
> typedef uint32_t uint32;
> typedef uint64_t uint64;
>
> #endif  // FST_LIB_TYPES_H__
>
> This is what the error happens with. The typedefs do not help.
>
> However, the code below, using Yenda's fix, compiles:
>
> #include "fst/fstlib.h"
> #include "fstext/fstext-lib.h"
> namespace fst {
> typedef ::int64 int64;
> }
> int main(int argc, char *argv[]) {
>   using namespace kaldi;
>   fst::VectorFst<fst::StdArc> phn2word;
>   fst::FstPrinter<fst::StdArc> fstprinter(phn2word, NULL, NULL, NULL, false, true);
> }
>
> Now try to explain that...
>
>  -kkm
>
>
>> -----Original Message-----
>> From: Daniel Povey [mailto:dp...@gm...]
>> Sent: 2015-05-19 1337
>> To: Kirill Katsnelson
>> Cc: kal...@li...
>> Subject: Re: [Kaldi-developers] Windows patches to src/ (mostly)
>>
>> I see now that the error is not what I thought.
>> I just checked the OpenFst code and Mike Riley did fix the issue; the
>> types are now the same as the Kaldi ones, assuming you are not on
>> Windows: OpenFst code says:
>>
>> #include <stdint.h>
>> ...
>> typedef int16_t int16;
>> typedef int32_t int32;
>> typedef int64_t int64;
>> etc
>>
>>
>> The problem is that the Kaldi code doesn't use stdint.h if using MSVC,
>> because at the time we started, stdint.h did not exist on Windows.
>> But I think removing that #ifdef, and using stdint.h, which now seems
>> to exist on Windows, should probably resolve the issue.  If ultimately
>> the typedef resolves to the same thing, I think the compiler doesn't
>> mind.
>> Dan
>>
>>
>>
>>
>> On Tue, May 19, 2015 at 4:30 PM, Kirill Katsnelson
>> <kir...@sm...> wrote:
>> > I am going back to reproduction the compile error in MSVC. I am using
>> bin/phones-to-prons as a simple example and have reduced the module to
>> the following exact lines:
>> >
>> > #include "fst/fstlib.h"
>> > #include "fstext/fstext-lib.h"
>> > int main(int argc, char *argv[]) {
>> >   using namespace kaldi;
>> >   fst::VectorFst<fst::StdArc> phn2word;
>> >   fst::FstPrinter<fst::StdArc> fstprinter(phn2word, NULL, NULL, NULL,
>> > false, true);  // line 6 }
>> >
>> > The error message follows. Only MSVC reports it, the Intel compiler
>> has no problem here:
>> >
>> > C:\projects\kaldi\kaldi\extlibs\openfstwin-
>> 1.3.1\include\fst/script/print-impl.h(77): error C2872: 'int64' :
>> ambiguous symbol [C:\projects\kaldi\kaldi\msbuild\bin\kaldi-bin.kwproj]
>> >           could be 'C:\projects\kaldi\kaldi\extlibs\openfstwin-
>> 1.3.1\include\fst/types.h(31) : int64_t int64'
>> >           or       'C:\projects\kaldi\kaldi\src\base/kaldi-
>> types.h(48) : kaldi::int64'
>> >           ..\../src/bin/phones-to-prons.cc(6) : see reference to
>> class
>> > template instantiation 'fst::FstPrinter<fst::StdArc>' being compiled
>> >
>> > The line print-impl.h(77), containing the reportedly ambiguous
>> symbol, is essentially this:
>> >
>> > namespace fst {
>> > template <class A> class FstPrinter {
>> >   void PrintId(int64 id, const SymbolTable *syms, const char *name)
>> const {  // Line 77
>> >   }
>> > };
>> > }
>> >
>> > This is indeed a compiler bug. The int64 does not even come from a
>> template parameter, and is unambiguously ::int64 in this scope, already
>> defined in an fst header.
>> >
>> > Now, you would think that the template is expanded in the lexical
>> scope of invocation, and thus preventing kaldi::int64 from being known
>> as just int64 at this point would be the fix. Yes it is a fix indeed,
>> but also is putting everything into the kaldi namespace. This compiles
>> without an error (then the linker complains about the missing _main,
>> but this is not a problem at this point):
>> >
>> > #include "fst/fstlib.h"
>> > #include "fstext/fstext-lib.h"
>> > namespace kaldi {
>> > int main(int argc, char *argv[]) {
>> >   fst::VectorFst<fst::StdArc> phn2word;
>> >   fst::FstPrinter<fst::StdArc> fstprinter(phn2word, NULL, NULL, NULL,
>> > false, true); } }
>> >
>> > Why this works I do not know. Perhaps, kaldi::int64 is used in the
>> template expansion. But it is also a fix (or, better put, a
>> workaround).
>> >
>> > I agree that the fst library goes way too far when defining global
>> types int64 and friends (fst/types.h). But I do not know if it is
>> possible to change at this point. If it is redefined as "fst::int64",
>> kaldi is going to have trouble in modules having both "using namespace
>> kaldi" and "using namespace fst", I think.
>> >
>> > An amazingly ugly fix would be to define int64 as an alias to
>> kaldi::int64 etc., and then #define FST_LIB_TYPES_H__ before including
>> fst headers, thus preventing fst's definitions. I certainly would not
>> like to depend on an internal detail of the fst library though.
>> >
>> >  -kkm
>> >
>> >> -----Original Message-----
>> >> From: Daniel Povey [mailto:dp...@gm...]
>> >> Sent: 2015-05-18 2011
>> >> To: Kirill Katsnelson
>> >> Cc: kal...@li...
>> >> Subject: Re: [Kaldi-developers] Windows patches to src/ (mostly)
>> >>
>> >> Hm.
>> >> If the only issue is the int32and int64 definitions, I think a
>> >> cleaner fix would be to apply a patch to OpenFst to make it use the
>> >> same definitions that Kaldi uses (which is the "proper" way to do
>> it).
>> >> Dan
>> >>
>> >>
>> >> On Mon, May 18, 2015 at 11:05 PM, Kirill Katsnelson
>> >> <kir...@sm...> wrote:
>> >> > I am not exactly "moving" main() into namespace kaldi: there is
>> >> another, global main that calls kaldi::main(). The kaldi::main() can
>> >> be named anything, and also I do not think the standard prohibits
>> >> defining functions called main in other namespaces.
>> >> >
>> >> > There are at least two ways to deal with the bug we discussed a
>> few
>> >> days ago:
>> >> > 1. Move everything into namespace kaldi. In this case, the
>> >> > namespace-
>> >> local definitions (e. g. kaldi::int64) always win. Indeed, I am
>> >> adding a trivial main() that calls kaldi::main() in each program.
>> >> > 2. Declare all used types explicitly. This is what I did in the
>> >> couple files there, see the long banners of the using statements.
>> >> Simply "using namespace kaldi" does not cut it.
>> >> >
>> >> > Which of the two ways to select is a question I do not have much
>> >> opinion on.
>> >> >
>> >> > The error I am getting is about functions taking kaldi and
>> >> > openfst's
>> >> declared types int32 and int64 by pointer or by reference being
>> >> different and incompatible. I do not remember the exact message, but
>> >> I can easily roll back the change and see, if it would make more
>> sense.
>> >> >
>> >> >  -kkm
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Daniel Povey [mailto:dp...@gm...]
>> >> >> Sent: 2015-05-18 1944
>> >> >> To: Kirill Katsnelson
>> >> >> Cc: kal...@li...
>> >> >> Subject: Re: [Kaldi-developers] Windows patches to src/ (mostly)
>> >> >>
>> >> >> Thanks a lot for these.
>> >> >> Yenda will deal with the checking-in issues.
>> >> >> Generally speaking I like your patches, but I'll just reiterate
>> >> >> here some comments I made on github:
>> >> >> I'm not happy with the way you worked around the MSVC compiler
>> bug
>> >> by
>> >> >> moving main() into namespace kaldi.  I believe this is not
>> >> >> allowed, e.g. see here
>> >> >> http://stackoverflow.com/questions/3956678/main-in-
>> >> >> namespace,
>> >> >> and even if it were allowed it seems very wrong to me; and I
>> doubt
>> >> it
>> >> >> would compile on all platforms.  I'm OK to deal with MSVC's
>> >> >> brokenness by adding various #ifdefs, but actually breaking Kaldi
>> >> >> code is where I draw the line.  I'm sure there must be less
>> >> >> heavyweight ways to deal with this problem?  I don't know what
>> >> >> error message it was that you got, that you are trying to get rid
>> of?
>> >> >>
>> >> >>
>> >> >> Dan
>> >> >>
>> >> >> On Mon, May 18, 2015 at 8:54 PM, Kirill Katsnelson
>> >> >> <kir...@sm...> wrote:
>> >> >> > I have collected all my source changes into 4 separate commits
>> >> >> > in my
>> >> >> git fork. Please review the changes at the following links:
>> >> >> >
>> >> >> > The first change is trivial, but is necessary for the whole rig
>> >> >> > to
>> >> >> work properly. EOL conventions are hard to fix later and cause
>> the
>> >> >> most confusion in version control.
>> >> >> > https://github.com/kaldi-asr/kaldi/commit/bba053
>> >> >> >
>> >> >> > Compatibility changes for Windows cl (VS2013) and icl (Intel XE
>> >> >> > 2015 Update 3) https://github.com/kaldi-asr/kaldi/commit/1cc30d
>> >> >> >
>> >> >> > This is a change to fix the apparent MSVC bug with namespace
>> >> >> > resolution in template expansion
>> >> >> > https://github.com/kaldi-asr/kaldi/commit/f0f9e7
>> >> >> >
>> >> >> > And the last deals with proper I/O binarization under Windows,
>> >> >> > and
>> >> >> Cygwin path conversions.
>> >> >> > The change depends on the 2nd from top above.
>> >> >> > https://github.com/kaldi-asr/kaldi/commit/5d2cc3
>> >> >> >
>> >> >> > There are also changes to openfst files and headers, but I do
>> >> >> > not
>> >> >> know how to deal with them:
>> >> >> >  - Check in the patches to be applied on top of existing
>> patches?
>> >> >> >  - Provide pre-build binaries?
>> >> >> >  - Check in complete sources as part of kaldi repo?
>> >> >> >
>> >> >> > And there are build scripts, coming soon.
>> >> >> >
>> >> >> > Thanks,
>> >> >> >
>> >> >> >  -kkm
>> >> >> >
>> >> >> > ---------------------------------------------------------------
>> -
>> >> >> > --
>> >> -
>> >> >> > --
>> >> >> -
>> >> >> > -------- One dashboard for servers and applications across
>> >> >> > Physical-Virtual-Cloud Widest out-of-the-box monitoring support
>> >> >> > with
>> >> >> > 50+ applications Performance metrics, stats and reports that
>> >> >> > 50+ give you
>> >> >> > Actionable Insights Deep dive visibility with transaction
>> >> >> > tracing using APM Insight.
>> >> >> > http://ad.doubleclick.net/ddm/clk/290420510;117567292;y
>> >> >> > _______________________________________________
>> >> >> > Kaldi-developers mailing list
>> >> >> > Kal...@li...
>> >> >> > https://lists.sourceforge.net/lists/listinfo/kaldi-developers