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.

Sign up for GitHub

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

Jump to bottom

Windows fixes #1537

Merged
merged 9 commits into from
May 30, 2024
Merged

Windows fixes #1537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c750749
MSVC: Remove the vcxproj.filters files
ampli May 29, 2024
fdf053a
LinkGrammar.vcxproj: Update the list of source file names
ampli May 29, 2024
f0c591e
table_count(): Bugfix !USE_TABLE_TRACON
ampli May 29, 2024
8703938
table_lookup(): Remove redundant check in assert()
ampli May 29, 2024
ed03595
table_lookup(): Print conflicting values in assert()
ampli May 29, 2024
dc9426b
table_lookup(): On !USE_TABLE_TRACON, return Count_bin type
ampli May 29, 2024
267dada
estimate_log2_table_size(): Avoid warning on conversion to/from double
ampli May 29, 2024
b4f22e4
Change GNUC_NORETURN to NORETURN and define for MSVC too
ampli May 30, 2024
3c23a1c
form_match_list(): Don't use _Atomic
ampli May 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,14 @@ EXTRA_DIST = \
msvc/LGlib-features.props \
msvc/LinkGenerator.vcxproj \
msvc/LinkGrammarExe.vcxproj \
msvc/LinkGrammarExe.vcxproj.filters \
msvc/LinkGrammarJava.vcxproj \
msvc/LinkGrammarJava.vcxproj.filters \
msvc/LinkGrammar.sln \
msvc/LinkGrammar.vcxproj \
msvc/LinkGrammar.vcxproj.filters \
msvc/Local.props \
msvc/confvar.bat \
msvc/MSVC-common.props \
msvc/post-build.bat \
msvc/Python3.vcxproj \
msvc/Python3.vcxproj.filters \
msvc/README.md \
msvc/make-check.py \
TODO
5 changes: 3 additions & 2 deletions link-grammar/error.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

#include "link-includes.h"
#include "externs.h" // verbosity
#include "utilities.h" // GNUC_NORETURN, STRINGIFY
#include "utilities.h" // NORETURN, STRINGIFY

/* User verbosity levels are 1-4, to be used for user info/debug.
* For now hard-coded numbers are still used instead of D_USER_BASIC/TIMES. */
Expand Down Expand Up @@ -97,8 +97,9 @@ void lg_lib_failure(void);

extern void (* assert_failure_trap)(void);
#define FILELINE __FILE__ ":" STRINGIFY(__LINE__)
NORETURN
void assert_failure(const char[], const char[], const char *, const char *, ...)
GNUC_PRINTF(4,5) GNUC_NORETURN;
GNUC_PRINTF(4,5);

/* Define a private version of assert() with a printf-like error
* message. The C one is not used. */
Expand Down
10 changes: 5 additions & 5 deletions link-grammar/parse/count.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,10 +576,10 @@ static Count_bin table_store(count_context_t *ctxt,
Count_bin *e = table_lookup(ctxt, lw, rw, le, re, null_count, NULL);
if (e != NULL)
{
assert((e == NULL) || (hist_total(&c) == hist_total(e)),
"Inconsistent count for w(%d,%d) tracon_id(%d,%d)",
lw, rw, l_id, r_id);
return c;
assert((hist_total(&c) == hist_total(e)),
"Inconsistent count for w(%d,%d) tracon_id(%d,%d): %zd != %zd",
lw, rw, l_id, r_id, (ssize_t)hist_total(&c), (ssize_t)hist_total(e));
return *e;
}

// The count is still stored, for the above consistency check
Expand Down Expand Up @@ -957,7 +957,7 @@ static Count_bin table_count(count_context_t * ctxt,
int lw, int rw, Connector *le, Connector *re,
unsigned int null_count)
{
if (!USE_TABLE_TRACON) return true;
if (!USE_TABLE_TRACON) return count_unknown;

/* This check is not necessary for correctness, but it saves CPU time.
* If a cross link would result, we know that the count would be 0.
Expand Down
7 changes: 4 additions & 3 deletions link-grammar/parse/extract-links.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,8 @@ static void record_choice(
static int estimate_log2_table_size(Sentence sent)
{
/* Size estimate based on measurements (see #1402) */
double lscale = log2(sent->num_disjuncts + 1.0) - 0.5 * log2(sent->length);
double lscale = log2((double)sent->num_disjuncts + 1.0) -
0.5 * log2((double)sent->length);
double lo_est = lscale + 4.0;
double hi_est = 1.5 * lscale;
double dj_est = fmax(lo_est, hi_est);
Expand All @@ -166,10 +167,10 @@ static int estimate_log2_table_size(Sentence sent)
* pex->Pset_bucket_pool is almost exactly equal to the num elts
* issued for sent->Table_tracon_pool. This provides a better
* estimate when parsing with MST, when the above is too low. */
double ntracon = pool_num_elements_issued(sent->Table_tracon_pool);
double ntracon = (double)pool_num_elements_issued(sent->Table_tracon_pool);
double ltra = log2(ntracon) + 1.0; // + 1.0 because floor()

int log2_table_size = floor(fmax(dj_est, ltra));
int log2_table_size = (int)floor(fmax(dj_est, ltra));

// Enforce min and max sizes.
if (log2_table_size < 4) log2_table_size = 4;
Expand Down
4 changes: 2 additions & 2 deletions link-grammar/parse/fast-match.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,8 +595,8 @@ form_match_list(fast_matcher_t *ctxt, int w,
}

#ifdef VERIFY_MATCH_LIST
static _Atomic(uint16_t) id = 0;
uint16_t lid = ++id; /* A local copy, for multi-threading support. */
static uint16_t id = 0;
uint16_t lid = ++id; /* A stable local copy, for multi-threading support. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will clearly fail when multi-threading, as the increment ++id can occur in two different threads (and so two threads get the same lid) which means that the assert at line 1470 of parse/count.c might trigger. I'm going to put this back, but then disable VERIFY_MATCH_LIST by default.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such a problem cannot happen, since each LG thread accesses only its own data structures!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in fac11ac

Copy link
Member Author

@ampli ampli May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the lid is put in the element list that a form_match_list() returns, and since it is an auto variable, all the elements of the list are guaranteed to get the same lid. When form_match_list() returns, the thread that has invoked it fetches the id from the first element and validates that the rest use the same id (to validate that the list has not been corrupted, as happens sometimes during development). So the code here doesn't care at all what lid values are used in other threads, and even not in the same thread (as long as lid is constant in a single invocation of form_match_list()`, which is guaranteed). They can be always the same, or any random ones.
The bug that got fixed then was due to an uninitialized id field on id==0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I skimmed the code and saw this:

link-grammar/link-grammar/parse/count.c

Line 1472 in c098940

assert(id == d->match_id, "Modified id (%u!=%u)", id, d->match_id);

and (without over-analyzing it) made me think that assert might now trigger by accident. So I got nervous about it. So I put the Atomic back in, and undef'ed VERIFY_MATCH_LIST so it becomes invisible.

Copy link
Member Author

@ampli ampli May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, my comment "for multi-threading support" just tells why the assignment to an auto variable lid is needed.
This is because a stable lid value is needed during the current form_match_list() call. (Hence I added "stable" to clarify it further.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go ahead and remove the _Atomic if you are sure. I wasn't going to think too hard about it.

#else
const uint16_t lid = 0;
#endif
Expand Down
11 changes: 7 additions & 4 deletions link-grammar/utilities.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,13 +253,13 @@ static inline char *_strndupa3(char *new_s, const char *s, size_t n)
* support C11. So it already supports all the features below. */

/* Optimizations etc. that only gcc understands */
/* FIXME: Change to ATTR_* and define also for MSVC. */
/* FIXME: Define also for MSVC. */
#if __GNUC__
#define GCC_DIAGNOSTIC
#define UNREACHABLE(x) (__extension__ ({if (x) __builtin_unreachable();}))
#define GNUC_MALLOC __attribute__ ((__malloc__))
#define GNUC_UNUSED __attribute__ ((__unused__))
#define GNUC_NORETURN __attribute__ ((__noreturn__))
#define NORETURN __attribute__ ((__noreturn__))
#define ATTR_PURE __attribute__ ((__pure__))
#define NO_SAN __attribute__ ((no_sanitize_address, no_sanitize_undefined))

Expand All @@ -271,7 +271,6 @@ static inline char *_strndupa3(char *new_s, const char *s, size_t n)
#else
#define NO_SAN_DICT
#endif

#ifndef DONT_EXPECT
#define likely(x) __builtin_expect(!!(x), 1)
#define unlikely(x) __builtin_expect(!!(x), 0)
Expand All @@ -281,14 +280,18 @@ static inline char *_strndupa3(char *new_s, const char *s, size_t n)
#define UNREACHABLE(x)
#define GNUC_MALLOC
#define GNUC_UNUSED
#define GNUC_NORETURN
#define NORETURN
#define ATTR_PURE
#define NO_SAN_DICT

#define likely(x) x
#define unlikely(x) x
#endif

#ifdef _MSC_VER
#undef NORETURN
#define NORETURN __declspec(noreturn)
#endif

/* Apply a pragma to a specific code section only.
* XXX According to the GCC docs, we cannot use here something like
Expand Down
42 changes: 29 additions & 13 deletions msvc/LinkGrammar.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,14 +235,15 @@
<ClInclude Include="..\link-grammar\api-structures.h" />
<ClInclude Include="..\link-grammar\api-types.h" />
<ClInclude Include="..\link-grammar\connectors.h" />
<ClInclude Include="..\link-grammar\dict-atomese\lookup-atomese.h" />
<ClInclude Include="..\link-grammar\dict-atomese\read-atomese.h" />
<ClInclude Include="..\link-grammar\dict-common\dialect.h" />
<ClInclude Include="..\link-grammar\dict-common\dict-affix.h" />
<ClInclude Include="..\link-grammar\dict-common\dict-affix-impl.h" />
<ClInclude Include="..\link-grammar\dict-common\dict-api.h" />
<ClInclude Include="..\link-grammar\dict-common\dict-common.h" />
<ClInclude Include="..\link-grammar\dict-common\dict-defines.h" />
<ClInclude Include="..\link-grammar\dict-common\dict-impl.h" />
<ClInclude Include="..\link-grammar\dict-common\dict-internals.h" />
<ClInclude Include="..\link-grammar\dict-common\dict-locale.h" />
<ClInclude Include="..\link-grammar\dict-common\dict-structures.h" />
<ClInclude Include="..\link-grammar\dict-common\dict-utils.h" />
<ClInclude Include="..\link-grammar\dict-common\file-utils.h" />
Expand All @@ -254,6 +255,10 @@
<ClInclude Include="..\link-grammar\dict-file\word-file.h" />
<ClInclude Include="..\link-grammar\dict-ram\dict-ram.h" />
<ClInclude Include="..\link-grammar\dict-sql\read-sql.h" />
<ClInclude Include="..\link-grammar\dict-atomese\dict-atomese.h" />
<ClInclude Include="..\link-grammar\dict-atomese\local-as.h" />
<ClInclude Include="..\link-grammar\dict-atomese\lookup-atomese.h" />
<ClInclude Include="..\link-grammar\dict-atomese\read-atomese.h" />
<ClInclude Include="..\link-grammar\disjunct-utils.h" />
<ClInclude Include="..\link-grammar\error.h" />
<ClInclude Include="..\link-grammar\externs.h" />
Expand All @@ -272,18 +277,18 @@
<ClInclude Include="..\link-grammar\parse\preparation.h" />
<ClInclude Include="..\link-grammar\parse\prune.h" />
<ClInclude Include="..\link-grammar\post-process\post-process.h" />
<ClInclude Include="..\link-grammar\post-process\pp-structures.h" />
<ClInclude Include="..\link-grammar\post-process\pp_knowledge.h" />
<ClInclude Include="..\link-grammar\post-process\pp_lexer.h" />
<ClInclude Include="..\link-grammar\post-process\pp_linkset.h" />
<ClInclude Include="..\link-grammar\post-process\pp-structures.h" />
<ClInclude Include="..\link-grammar\prepare\build-disjuncts.h" />
<ClInclude Include="..\link-grammar\prepare\exprune.h" />
<ClInclude Include="..\link-grammar\print\print-util.h" />
<ClInclude Include="..\link-grammar\print\print.h" />
<ClInclude Include="..\link-grammar\print\print-util.h" />
<ClInclude Include="..\link-grammar\print\wcwidth.h" />
<ClInclude Include="..\link-grammar\resources.h" />
<ClInclude Include="..\link-grammar\string-id.h" />
<ClInclude Include="..\link-grammar\string-set.h" />
<ClInclude Include="..\link-grammar\string-id.h" />
<ClInclude Include="..\link-grammar\tokenize\anysplit.h" />
<ClInclude Include="..\link-grammar\tokenize\spellcheck.h" />
<ClInclude Include="..\link-grammar\tokenize\tok-structures.h" />
Expand All @@ -295,13 +300,13 @@
<ClInclude Include="link-grammar\link-features.h" />
</ItemGroup>
<ItemGroup>
<ClCompile Include="..\link-grammar\api.c" />
<ClCompile Include="..\link-grammar\config.c" />
<ClCompile Include="..\link-grammar\connectors.c" />
<ClCompile Include="..\link-grammar\dict-atomese\lookup-atomese.cc" />
<ClCompile Include="..\link-grammar\dict-atomese\read-atomese.c" />
<ClCompile Include="..\link-grammar\dict-common\dialect.c" />
<ClCompile Include="..\link-grammar\dict-common\dict-affix-impl.c" />
<ClCompile Include="..\link-grammar\dict-common\dict-common.c" />
<ClCompile Include="..\link-grammar\dict-common\dict-impl.c" />
<ClCompile Include="..\link-grammar\dict-common\dict-internals.c" />
<ClCompile Include="..\link-grammar\dict-common\dict-locale.c" />
<ClCompile Include="..\link-grammar\dict-common\dict-utils.c" />
<ClCompile Include="..\link-grammar\dict-common\file-utils.c" />
<ClCompile Include="..\link-grammar\dict-common\idiom.c" />
Expand All @@ -312,12 +317,20 @@
<CompileAs Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">CompileAsCpp</CompileAs>
<CompileAs Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">CompileAsCpp</CompileAs>
</ClCompile>
<ClCompile Include="..\link-grammar\dict-file\dictionary.c" />
<ClCompile Include="..\link-grammar\dict-file\read-dialect.c" />
<ClCompile Include="..\link-grammar\dict-file\dictionary.c" />
<ClCompile Include="..\link-grammar\dict-file\read-dict.c" />
<ClCompile Include="..\link-grammar\dict-file\read-regex.c" />
<ClCompile Include="..\link-grammar\dict-file\word-file.c" />
<ClCompile Include="..\link-grammar\dict-ram\dict-ram.c" />
<ClCompile Include="..\link-grammar\dict-sql\read-sql.c" />
<ClCompile Include="..\link-grammar\dict-atomese\read-atomese.c" />
<ClCompile Include="..\link-grammar\dict-atomese\link-names.cc" />
<ClCompile Include="..\link-grammar\dict-atomese\lookup-atomese.cc" />
<ClCompile Include="..\link-grammar\dict-atomese\fetch-cats.cc" />
<ClCompile Include="..\link-grammar\dict-atomese\sections.cc" />
<ClCompile Include="..\link-grammar\dict-atomese\word-pairs.cc" />
<ClCompile Include="..\link-grammar\dict-atomese\utils.cc" />
<ClCompile Include="..\link-grammar\disjunct-utils.c" />
<ClCompile Include="..\link-grammar\error.c" />
<ClCompile Include="..\link-grammar\linkage\analyze-linkage.c" />
Expand All @@ -327,6 +340,7 @@
<ClCompile Include="..\link-grammar\linkage\sane.c" />
<ClCompile Include="..\link-grammar\linkage\score.c" />
<ClCompile Include="..\link-grammar\memory-pool.c" />
<ClCompile Include="..\link-grammar\options.c" />
<ClCompile Include="..\link-grammar\parse\count.c" />
<ClCompile Include="..\link-grammar\parse\extract-links.c" />
<ClCompile Include="..\link-grammar\parse\fast-match.c" />
Expand All @@ -341,13 +355,15 @@
<ClCompile Include="..\link-grammar\post-process\pp_linkset.c" />
<ClCompile Include="..\link-grammar\prepare\build-disjuncts.c" />
<ClCompile Include="..\link-grammar\prepare\exprune.c" />
<ClCompile Include="..\link-grammar\print\print-util.c" />
<ClCompile Include="..\link-grammar\print\print.c" />
<ClCompile Include="..\link-grammar\print\print-util.c" />
<ClCompile Include="..\link-grammar\print\wcwidth.c" />
<ClCompile Include="..\link-grammar\resources.c" />
<ClCompile Include="..\link-grammar\string-id.c" />
<ClCompile Include="..\link-grammar\sentence.c" />
<ClCompile Include="..\link-grammar\string-set.c" />
<ClCompile Include="..\link-grammar\string-id.c" />
<ClCompile Include="..\link-grammar\tokenize\anysplit.c" />
<ClCompile Include="..\link-grammar\tokenize\lookup-exprs.c" />
<ClCompile Include="..\link-grammar\tokenize\spellcheck-aspell.c" />
<ClCompile Include="..\link-grammar\tokenize\spellcheck-hun.c" />
<ClCompile Include="..\link-grammar\tokenize\tokenize.c" />
Expand Down Expand Up @@ -394,4 +410,4 @@
<ImportGroup Label="ExtensionTargets" Condition="'$(WINFLEXBISON)'!=''">
<Import Project="$(WINFLEXBISON)\custom_build_rules\win_flex_only\win_flex_custom_build.targets" />
</ImportGroup>
</Project>
</Project>
Loading