change size_t to uint for OSX 64 bit

[WalterBright]

Because of:

Undefined symbols for architecture x86_64:
  "Identifier::idPool(char const*, unsigned long)", referenced from:
      test_visitors() in cxxfrontend.o
      test_semantic() in cxxfrontend.o

https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=3165089&isPull=true

[dlang-bot]

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8249"

[WalterBright]

Blocking #8112

[jacob-carlborg]

Is this the best solution?

[WalterBright]

There are problems with any solution due to bootstrapping issues with older and newer dmd's. The easiest solution is avoiding the issue, which is what this PR does.

[JinShil]

I don't mind these kinds of workarounds as long as there is a documented plan on how they are going to be backed out.

Will this workaround only be needed until a certain version of the host compiler is used? If so, add a bugzilla issue, and put a // FIXME: Issue 12345 next to the lines of code that need to be backed out when the time is right. Otherwise, we end up with these hacks lingering in the code forever with noone who remembers why they are there.

[WalterBright]

we end up with these hacks lingering in the code forever with noone who remembers why they are there.

That's why there's git blame. Besides, these changes are completely trivial.

[JinShil]

That's why there's git blame. Besides, these changes are completely trivial.

Sorry, that's not good enough, if you want my approval.

[jacob-carlborg]

That's why there's git blame. Besides, these changes are completely trivial.

Your commit message doesn’t contain any reason for this change. Unfortunately we’re very bad on this in general.

[schveiguy]

I agree with @JinShil here. We should change this back eventually. Not because I think it's possible to have longer than uint.max length string identifiers, but because it's a pain to deal with parameters that mean 'length' that aren't size_t in D, especially when the underlying calls convert back to size_t. The end result is that the intermediate compiler will have to move from 2.079 to something that deals with it.

So what happens when we do remove this hack? I would like to see that we still only need a 2-stage build process, which means we don't have much time to fix this (probably one or 2 releases). Can we get the true fixes for the problem into the stable branch of 2.080? Or 2.081?

[kinke]

Is this the best solution?

Definitely not. The long mangling change with 2.079 for macOS seemed super-arbitrary and was a big mistake IMO. So reverting that and instead introducing cpp_uint64_t (=> magic cpp_ulonglong enum) is what I'd suggest (and implemented by LDC 1.10). size_t is pretty ubiquitous, whereas C++ unsigned long long (uint64_t on 64-bit macOS) isn't.

[WalterBright]

Your commit message doesn’t contain any reason for this change.

See the top comment.

[WalterBright]

The long mangling change with 2.079 for macOS seemed super-arbitrary

True or not, that is irrelevant to this PR. This PR is to get the dmd source code to compile independently of the vagaries of size_t mangling.

[WalterBright]

So what happens when we do remove this hack?

The same thing that happens now. It won't compile on OSX because OSX's size_t is an unsigned long which mangles differently.

probably one or 2 releases

How many more years do we need to wait before the back end can be converted to D?

[kinke]

The long mangling change with 2.079 for macOS seemed super-arbitrary

True or not, that is irrelevant to this PR. This PR is to get the dmd source code to compile independently of the vagaries of size_t mangling.

This PR is exactly the kind of ugly adjustments your 2.079 mangling change imposes on all mixed D/C++ code bases. I think there's still time to revert it; DMD 2.079 was bad enough, with C++ size_t not even being representable in D without manually declaring a magic struct (!) on 64-bit macOS.

[WalterBright]

@kinke you might like dlang/druntime#2183 then.

Fixing name mangling is not what this PR is about. It is about getting dmd to compile on the Mac so I can proceed with converting the back end to D. Delaying it for yet another year over trivia like a couple casts is unproductive.

[jacob-carlborg]

See the top comment.

The PR description is not the same as the commit message. If you write the commit message with a title, an empty newline followed by a longer description GitHub will automatically use that for the title and description of the PR.

[schveiguy]

The same thing that happens now.

So is there no intention to fix the mangling issues? Are they unfixable?

How many more years do we need to wait before the back end can be converted to D?

I don't think you realize what I meant. If we put this in, we will likely want to revert the changes once the base compiler can handle it -- it's annoying to have to cast down to uint when you have a size_t, especially when the real thing (stringtable.update, and Identifier constructor) uses a size_t. I'm not saying we don't pull this, what I was saying is that if we pull it, we should have a plan to truly fix the issues, and then standardize on the fixed compiler as the base.

BUT, if we wait too long, the compiler will naturally start using more and more features of the newer compiler that CAN'T be built using 2.068.2. At that point, we will need a 3-stage build process -> use 2.068.2 to build 2.079.0, then use 2.079.0 to build 2.xxx (the version which handles the mangling properly), and then use that to build master. If we fix the mangling issues soon, then it still remains a 2-stage, and we just bump the base compiler from 2.079 to 2.xxx.

Of course, we can just keep the casts in, and live with this oddity forever. In which case, PLEASE put in a comment on the function prototype so people don't remove them (would be good to reference a bug report there as well).

[WalterBright]

So is there no intention to fix the mangling issues?

They're not unfixable. They just aren't fixed yet. All this PR does is avoid using size_t so the source code is independent of those issues. Such independence from size_t makes the source code more portable among dmd versions, not less.

Waiting yet another year for future versions, and then getting GDC and LDC to fold them in, just to avoid a couple casts is not reasonable. We'd never get anywhere sticking to such a philosophy.

BTW, I also have submitted a couple of PRs that do fixes for mangling, but nothing is happening with them, either.

[JinShil]

So reverting that and instead introducing cpp_uint64_t (=> magic cpp_ulonglong enum) is what I'd suggest (and implemented by LDC 1.10). size_t is pretty ubiquitous, whereas C++ unsigned long long (uint64_t on 64-bit macOS) isn't.

Is there any reason why we can't do this?

[WalterBright]

Is there any reason why we can't do this?

It'll delay this PR for several release cycles.

[ibuclaw]

What change triggered this link error exactly? The linked test failure looks like an unrelated druntime change dlang/druntime#2151.

While it's nice that the C++ binding test caught this, I don't think we are even close to testing enough of the frontend, meaning there are probably far more extern(C++) functions that need to be updated from d_size_t to uint if we want ourselves covered.

[WalterBright]

What change triggered this link error exactly?

The mangling of size_t on OSX is different in C++ than in D.

there are probably far more

If there were, there would be more link failures. I've removed dependencies on size_t already in numerous other places, for the same reason. It doesn't work with OSX. This PR removes a new dependency that was added.

Test cases for the compiler belong in the test suite, not in the compiler itself.

[ibuclaw]

If there were, there would be more link failures.

The current cxxfrontend test only uses a few of the exposed functions, and in no way is representative of an actual project written in C++ that links to dmd, such as what gdc and ldc does.

You won't get a link error unless you call it from C++.

Test cases for the compiler belong in the test suite, not in the compiler itself.

Indeed, but a discrepancy between mangling does not mean its a compiler bug. The cxxfrontend test is there to catch implementation bugs only.

[ibuclaw]

I'm not saying no, but I am concerned there is more hiding. All extern(C++) functions that accept or return d_size_t should be checked.

[kinke]

FWIW, this is how I handle this mess in LDC: ldc-developers/ldc#2704
I.e., adapting C++ d_size_t according to whether the used host D compiler is DMD >= 2.079, and switching from size_t do d_size_t in 1-2 places in the C++ headers to fix the linking errors, not touching D files at all. I still count 82 occurrences of size_t in DMD C++ headers included in LDC, most of them (function params and return types) would need to be replaced by d_size_t to fix all potential linking errors.

[WalterBright]

@kinke complex additions to the makefile can make this work, but the makefiles are already fragile and excessively complex. I prefer the simpler solution of just avoiding using size_t until the boot compiler mangles size_t correctly, which should be in another year (at the current rate of progress). And there's still that awful d_size_t.

@ibuclaw I really don't understand what your concern is or why it would impede pulling this PR.

[kinke]

I prefer the simpler solution of just avoiding using size_t until the boot compiler mangles size_t correctly

Huh? It's always been mangled correctly, only the 2.079 OSX mangling broke the interop, so how is that supposed to fix things in a year?

And there's still that awful d_size_t.

Not still, but new and only required because of the mangling change. We'd have none of these issues without that change. As soon as my cpp_longlong PRs (dlang/druntime#2187 and #8262) are merged, I'll work on a PR reverting to the previous mangling. That would obsolete this PR (if simply not supporting 2.079 and 2.080 host compilers on 64-bit macOS to keep makefile logic simple).

[WalterBright]

It's always been mangled correctly

If it was there would never have been a d_size_t.

That would obsolete this PR

No, it wouldn't. I think there's a misunderstanding on what this PR is for. It is to enable moving the boot compiler version forward so I can convert the backend from C++ to D. Delaying it for future compiler versions means delaying the backend conversion likely for another year.

[kinke]

Well in that case I give up the resistance, it's just a single function and a couple of call sites as a hack to make the superficial C++ interop tests happy, so no need to make this bigger than it is. Just try not to convert any more size_t's to uint (with ugly casts) please. The mangling discussion can be postponed to my PR then. ;)

[WalterBright]

Thank you. BTW, you'll find these same casts elsewhere in the dmd source code, and nobody raised an eyebrow :-)

[JinShil]

Isn't it true that this can be reverted in a year? If so, please add a comment next to each cast to remind maintainers to do so in the future.

[WalterBright]

Isn't it true that this can be reverted in a year?

Only if all stakeholders agree on an upgraded boot compiler.

If so, please add a comment next to each cast to remind maintainers to do so in the future.

Such comments are superfluous because the reason for the cast is pretty obvious by inspection. Neither does anyone need reminding. It's like reminding people to add in const at a later date. Reminding does no good, I've suggested adding const endlessly, and nobody does. I wrote a whole post on the n.g. with such suggestions - no result.

Anyhow, the reason that cast uses a keyword in D, unlike in C, is so that they stand out. Casts are always a bit of a code smell, and anyone looking to remove technical debt can easily start by searching for casts.

And lastly, even with the casts in this PR, the code is not broken unless you're worried about 4Gb strings in DMD.

[JinShil]

I don't know how else to resolve this stalemate: https://issues.dlang.org/show_bug.cgi?id=18883

[jacob-carlborg]

How about using pragma(mangle)? Then size_t can still be used.

[andralex]

@WalterBright code is fine. Better yet just use ulong in the signature and insert a comment to the effect of "// Using ulong instead of size_t to work around OSX's special mangling of size_t". That's one line changed and one comment added. Done and done.

Thanks @JinShil for the resolution anyway. This discussion is woefully disproportionate compared to the matter at hand.