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

CAMLlocalN does not compile with msvc compiler #7581

Closed
vicuna opened this issue Jul 11, 2017 · 12 comments
Closed

CAMLlocalN does not compile with msvc compiler #7581

vicuna opened this issue Jul 11, 2017 · 12 comments
Assignees
Milestone

Comments

@vicuna
Copy link

vicuna commented Jul 11, 2017

Original bug ID: 7581
Reporter: @nojb
Status: acknowledged (set by @xavierleroy on 2017-07-12T17:07:22Z)
Resolution: open
Priority: normal
Severity: minor
Target version: 4.07.0+dev/beta2/rc1/rc2
Category: runtime system and C interface
Monitored by: @ygrek @oandrieu

Bug description

The msvc compiler allows variable declarations only after opening braces. As such the CAMLlocalN macro does not errors out.

Is there some C macro wizardy that would make this work ?

@vicuna
Copy link
Author

vicuna commented Jul 12, 2017

Comment author: @yallop

The difficulty is apparently caused by the loop that initializes 'x':

  for (caml__i_##x = 0; caml__i_##x < size; caml__i_##x ++) { \
    x[caml__i_##x] = Val_unit; \
  } \

CAMLlocalN inserts the output of CAMLxparamN (i.e. some additional declarations) after the loop, but this kind of mixing of declarations and code is not allowed in C89.

One possible fix is to move the array initialization code to an inline function (assuming it can be guaranteed that the function is actually always inlined) and replace the loop by a dummy declaration whose rhs calls the function:

  CAMLunused_start int caml__dummy_##x = (caml_initialize_array(x, size), 0);

@vicuna
Copy link
Author

vicuna commented Jul 12, 2017

Comment author: @xavierleroy

Would it be possible to initialize the array with zeros instead of Val_unit? (I think it's OK with the current GC but maybe not with the no-naked-pointer stuff.) In this case,

int x[N] = {0, };

is enough to initialize the local array x with all zeroes, regardless of its size N.

@vicuna
Copy link
Author

vicuna commented Jul 12, 2017

Comment author: @xavierleroy

Wading through history, I see zero-initialization was used (correctly) until commit 05100e5 and following, which insisted on Val_unit initialization.

@vicuna
Copy link
Author

vicuna commented Jul 18, 2017

Comment author: @nojb

From Github's discussion: this issue only affects versions prior to Visual 2013.

@vicuna
Copy link
Author

vicuna commented Jul 18, 2017

Comment author: @stedolan

It's also possible to just add some more curly braces. C89 insists that declarations come at the start of a block, not necessarily at the start of a function.

Something like this should work:

{
int caml__i_##x;
for (caml__i_##x = 0; caml__i_##x < size; caml__i_##x ++) {
x[caml__i_##x] = Val_unit;
}
}

@vicuna
Copy link
Author

vicuna commented Jul 18, 2017

Comment author: @nojb

This works if CAMLlocalN is the last declaration in the enclosing function.

@vicuna
Copy link
Author

vicuna commented Jul 26, 2017

Comment author: @stedolan

This works if CAMLlocalN is the last declaration in the enclosing function.

Of course, you're right. I think yallop's solution works in all cases, as does zero-initialisation (with the appropriate check in caml_do_local_roots).

@vicuna
Copy link
Author

vicuna commented Oct 15, 2017

Comment author: @xavierleroy

We need to decide a course of action.

One possibility is to partially revert 05100e5 and following commits by supporting (again) zero-initialization of local roots (but only local roots). What this means is just an extra test "root != NULL" in the "Local C roots" part of [caml_oldify_local_roots].

Another possibility is to say that versions of MSVC prior to Visual 2013 are no longer officially supported.

@vicuna
Copy link
Author

vicuna commented Oct 20, 2017

Comment author: @mshinwell

@lpw25 and I were discussing adding an explicit Obj.null value (which would usually be represented by an actual zero). Doing this would necessitate the GC being safe against it even in no-naked-pointers mode. That value could then be used for the initialisation of the arrays in this issue.

For the moment I would suggest trying the inline function version, although I don't think you will be obtain a guarantee that the function is inlined.

@vicuna vicuna added the stdlib label Mar 14, 2019
@vicuna vicuna added this to the 4.07.0 milestone Mar 14, 2019
@vicuna vicuna added the bug label Mar 20, 2019
@mshinwell
Copy link
Contributor

I think we're planning to produce a patch with an explicit Obj.null in the near future, so the zero-initialisation approach could then be used here.

@mshinwell mshinwell self-assigned this Apr 17, 2020
@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@damiendoligez
Copy link
Member

ISTR that we stopped restricting ourselves to C89 some time ago. For example, there are a number of // comments in the C code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants