Menu
▾
▴
Re: [Kaldi-developers] Windows patches to src/ (mostly)
|
From: Kirill K. <kir...@sm...> - 2015-05-19 21:20:21
|
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
|
