Fix exported names from gc.c to have jl_gc_ prefix #11741

ScottPJones · 2015-06-18T03:03:22Z

Changed exported names in gc.c to have the jl_gc_ prefix to prevent name collisions when julia is linked with other code.

gc_wb* -> jl_gc_wb*
gc_queue_root -> jl_gc_queue_root
allocobj -> jl_gc_allocobj
alloc_[0-4]w -> jl_gc_alloc_*w
diff_gc_total_bytes -> jl_gc_diff_total_bytes
sync_gc_total_bytes -> jl_gc_sync_total_bytes

yuyichao · 2015-06-18T03:06:06Z

yuyichao% git grep gc_wb doc | wc -l
4

ScottPJones · 2015-06-18T03:11:04Z

@yuyichao You were the one that pointed to the gc_wb functions 😀 Please tell me what else you've seen that I need to change!

yuyichao · 2015-06-18T03:12:55Z

You were the one that pointed to the gc_wb functions

And I said that it is in the embedding doc #11363 (comment).

Just do a git grep on the function you changes and make sure to change it everywhere, not limited to c code. (especially the doc)

yuyichao · 2015-06-18T03:17:24Z

src/gc.c

@@ -785,21 +785,21 @@ static inline int maybe_collect(void)

 // preserved values

-DLLEXPORT int jl_gc_n_preserved_values(void)
+int jl_gc_n_preserved_values(void)


These were the ones I asked about originally, that were exported, but not used anywhere in Base or registered packages. I wasn't aware of the embedded document when I started... these functions aren't described there, do they need to be exported?

At least I think they are potentially useful and shouldn't cause any issues to be exported.

OK, I'll put the DLLEXPORTs back on thos... I just don't like having unused stuff cluttering up an API...

ScottPJones · 2015-06-18T03:32:06Z

And as you are there, there's also gc_wb and friends

Ah, sorry, I missed the later comment you made - I was just going by the above comment!
I wasn't using git grep, I was using my editor's file search/replace function... missed the .rst file extension... I've gone back and looked at absolutely every file in the repository... I did catch all occurrences now. I even found something that hadn't been updated in the documentation...
jl_gc_enable()/jl_gc_disable() are now jl_gc_enable(1)/jl_gc_enable(0).

tkelman · 2015-06-18T04:04:37Z

src/julia.h


 #endif

-DLLEXPORT void *jl_gc_managed_malloc(size_t sz);
-DLLEXPORT void *jl_gc_managed_realloc(void *d, size_t sz, size_t oldsz, int isaligned, jl_value_t* owner);
+void *jl_gc_managed_malloc(size_t sz);


This still has DLLEXPORT at the implementation in gc.c, at least with MSVC the linkage needs to be consistent. I'd err on the side of leaving this exported, since someone saw the need to put DLLEXPORT on it in the first place.

Already fixed, just waiting for Base.runtests() to complete again!

ScottPJones · 2015-06-18T04:07:06Z

The change to jl_gc_enable was one of @JeffBezanson's, 8ff36e1
I guess he forgot to update the documentation

JeffBezanson · 2015-06-18T04:19:54Z

This is great, thanks for taking care of it.

ScottPJones · 2015-06-18T04:22:43Z

I may have to pull out the update for jl_gc_enable to another PR though, since this one is a breaking change for embedders, apparently, 😞 so who knows when it might ever get merged!

JeffBezanson · 2015-06-18T04:43:20Z

doc/manual/embedding.rst

@@ -196,8 +196,8 @@ There are some functions to control the GC. In normal use cases, these should no

 ========================= ==============================================================================
 ``void jl_gc_collect()``   Force a GC run
-``void jl_gc_disable()``   Disable the GC
-``void jl_gc_enable()``    Enable the GC
+``void jl_gc_enable(0)``   Disable the GC


Returns int.

Ah, right! Will fix.

still needs fixing?

JeffBezanson · 2015-06-18T04:43:44Z

I'm not too worried about that. I would merge this.

waTeim · 2015-06-18T05:24:02Z

I am in the clear on this one anyway (whew!). Are there others?

ScottPJones · 2015-06-18T07:30:34Z

@waTeim The initial message gives the full list that I changed.
The idea was actually to make life easier for people embedding, or linking in large C libraries!

ScottPJones · 2015-06-18T08:17:38Z

OK, I've fixed my stupid mistake correcting the documentation of jl_gc_enable!

ScottPJones · 2015-06-18T11:19:16Z

I found these also, that weren't from gc.c (which is what this PR was about cleaning up):
What should be done with them? Thanks!
show_execution_point
_resetstkoflw
___chkstk_ms
__chkstk
_chkstk
_alloca

pao · 2015-06-18T12:38:38Z

I think some (all?) of those are entry points for LLVM so we can't rename them, see #9339 (comment). But @vtjnash would know for sure.

ScottPJones · 2015-06-18T12:56:24Z

@pao Yes, I noticed them afterwards in a bunch of ../dep/llvm* files...
This is another place where a little internal documentation would go a long way!
It does look like show_execution_point should be prefixed with jl_ as well...

ScottPJones · 2015-06-18T12:59:35Z

Or is show_execution_point some special name also? For a debugger? (again, internal documentation saves time)

vtjnash · 2015-06-18T15:53:01Z

git blame says it was added by @timholy in #7464, but doesn't appear to get used?

the _ prefix on those other variables indicates that they come from the compiler

ScottPJones · 2015-06-18T16:06:12Z

@vtjnash About the _ indicating they came from the compiler... that's just a convention... which I was aware of, however, which compiler? Julia is a compiler... A little internal documentation can go a long way... (just saying that those names are required by LLVM).

ScottPJones · 2015-06-18T16:10:26Z

All I can find (in all of the JuliaLang/julia repository) is the definition, and the setup of the name for LLVM.

/j/julia/src/julia.h: void show_execution_point(char *filename, int lno);
/j/julia/src/codegen.cpp: static Function *show_execution_point_func;
/j/julia/src/codegen.cpp:     show_execution_point_func =
/j/julia/src/codegen.cpp:                          "show_execution_point", m);
/j/julia/src/codegen.cpp:     add_named_global(show_execution_point_func, (void*)*show_execution_point);
/j/julia/src/debuginfo.cpp: void show_execution_point(char *filename, int lno)

vtjnash · 2015-06-18T16:29:08Z

however, which compiler? Julia is a compiler...

julia's compiler uses jl_ as it's prefix, as you noted at the top

ScottPJones · 2015-06-18T16:32:44Z

Unfortunately, as I've shown in this PR, not consistently, which is why I was asking about those in the first place!

ScottPJones · 2015-06-18T16:35:35Z

There are also many many "internal" functions, that don't follow the jl_ convention, and I think those should also be fixed, but I think just dealing with the exported ones, which can break things, was enough for a little programming while watching the TV! 😀

tkelman · 2015-06-18T22:36:06Z

doc/manual/embedding.rst

-``void jl_gc_collect()``   Force a GC run
-``void jl_gc_disable()``   Disable the GC
-``void jl_gc_enable()``    Enable the GC
+``void jl_gc_collect()``  Force a GC run


rst might be picky about the spacing here, need to check

I did a full install build, and the documentation looked fine to me... did you see a problem?

as long as the table looks right in the html-rendered version then should be fine

Keno · 2015-06-18T23:31:23Z

Since it's breaking, this probably deserves a NEWS entry. Other than that this looks fine to me.

ScottPJones · 2015-06-18T23:47:34Z

OK, very good point! Will do.

JeffBezanson · 2015-06-19T04:18:44Z

👍 LGTM.

ScottPJones · 2015-06-19T05:25:48Z

I've updated NEWS.md, but I noticed something else while updating it,
@JeffBezanson: there's no mention of the recent Union()-> Union{} change.
Shouldn't that also be in NEWS.md? Thanks!

ScottPJones · 2015-06-19T11:22:31Z

@tkelman OK, changed to a sublist!

tkelman · 2015-06-19T16:41:53Z

Thanks, that looks better, but there's still an unaddressed comment, and needs a rebase probably due to your other news update

ScottPJones · 2015-06-19T17:37:00Z

What unaddressed comment? Am I missing something?

tkelman · 2015-06-19T18:26:54Z

#11741 (comment) - may have gotten lost in a rebase?

yuyichao · 2015-06-19T20:44:43Z

I've also had a similar comment. In additional to the void return type being wrong, (0) and (1) are not valid signature either. Probably should merge the two lines.

ScottPJones · 2015-06-19T21:11:01Z

Yep, I lost that change, because I edited in directly on GitHub, and then when I next pushed...

Put back some DLLEXPORT

ScottPJones · 2015-06-20T00:18:53Z

@yuyichao What's wrong with the (0) and (1)? That seemed to be the best way to describe it... it takes an int, 0 to disable, 1 (or non-zero) to enable.
@tkelman I put back my documentation change, squashed out the typo fixes

yuyichao · 2015-06-20T00:23:27Z

(0) and (1) themselves are fine, but not with return types, because it's invalid code.

The functions in the doc is either shown by how they are used, or by their signature e.g. this

Use sublist in NEWS.md

ScottPJones · 2015-06-20T02:17:56Z

@yuyichao, let me know if you think the way I changed the documentation seems OK to you, thanks!

yuyichao · 2015-06-20T02:42:31Z

I personally prefer listing the signature but this is also good for me.

ScottPJones · 2015-06-20T02:45:09Z

The signature didn't give what the int return value meant, so I thought this way might be more informative, that's all.

JeffBezanson · 2015-06-20T02:51:21Z

All looks good to me.

ScottPJones · 2015-06-20T19:14:00Z

Anybody willing to merge this? (I'd love to pare down my plethora of outstanding PRs before Wednesday! 😀)

Fix exported names from gc.c to have jl_gc_ prefix

ScottPJones · 2015-06-20T21:45:05Z

Thanks!

tkelman added breaking This change will break code embedding Embedding Julia using the C API labels Jun 18, 2015

yuyichao reviewed Jun 18, 2015
View reviewed changes

tkelman reviewed Jun 18, 2015
View reviewed changes

JeffBezanson reviewed Jun 18, 2015
View reviewed changes

tkelman reviewed Jun 18, 2015
View reviewed changes

tkelman removed the breaking This change will break code label Jun 18, 2015

ScottPJones force-pushed the spj/fixnames branch from 072b9eb to 0f13690 Compare June 19, 2015 05:18

ScottPJones added 2 commits June 19, 2015 19:57

Fix exported names from gc.c to have jl_gc_ prefix

7f0deec

Update documentation

04b58e0

Put back some DLLEXPORT

ScottPJones force-pushed the spj/fixnames branch from 943a9ea to 9c90e05 Compare June 20, 2015 00:14

ScottPJones added 2 commits June 19, 2015 20:51

Fix table

0bfa19f

Update NEWS.md

a89f203

Use sublist in NEWS.md

ScottPJones force-pushed the spj/fixnames branch from 9c90e05 to a89f203 Compare June 20, 2015 00:54

JeffBezanson added a commit that referenced this pull request Jun 20, 2015

Merge pull request #11741 from ScottPJones/spj/fixnames

c6ceac2

Fix exported names from gc.c to have jl_gc_ prefix

JeffBezanson merged commit c6ceac2 into JuliaLang:master Jun 20, 2015

tkelman mentioned this pull request Oct 17, 2015

Export strtod_c/strtof_c from src/ #13641

Merged

Fix exported names from gc.c to have jl_gc_ prefix #11741

Fix exported names from gc.c to have jl_gc_ prefix #11741

Conversation

ScottPJones commented Jun 18, 2015

yuyichao commented Jun 18, 2015

ScottPJones commented Jun 18, 2015

yuyichao commented Jun 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScottPJones commented Jun 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ScottPJones commented Jun 18, 2015

JeffBezanson commented Jun 18, 2015

ScottPJones commented Jun 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Jun 18, 2015

waTeim commented Jun 18, 2015

ScottPJones commented Jun 18, 2015

ScottPJones commented Jun 18, 2015

ScottPJones commented Jun 18, 2015

pao commented Jun 18, 2015

ScottPJones commented Jun 18, 2015

ScottPJones commented Jun 18, 2015

vtjnash commented Jun 18, 2015

ScottPJones commented Jun 18, 2015

ScottPJones commented Jun 18, 2015

vtjnash commented Jun 18, 2015

ScottPJones commented Jun 18, 2015

ScottPJones commented Jun 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keno commented Jun 18, 2015

ScottPJones commented Jun 18, 2015

JeffBezanson commented Jun 19, 2015

ScottPJones commented Jun 19, 2015

ScottPJones commented Jun 19, 2015

tkelman commented Jun 19, 2015

ScottPJones commented Jun 19, 2015

tkelman commented Jun 19, 2015

yuyichao commented Jun 19, 2015

ScottPJones commented Jun 19, 2015

ScottPJones commented Jun 20, 2015

yuyichao commented Jun 20, 2015

ScottPJones commented Jun 20, 2015

yuyichao commented Jun 20, 2015

ScottPJones commented Jun 20, 2015

JeffBezanson commented Jun 20, 2015

ScottPJones commented Jun 20, 2015

ScottPJones commented Jun 20, 2015