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

ldc2.conf: Implement ~= concatenating operator #4848

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

the-horo
Copy link
Contributor

The main goal that I'm trying to achieve with these changes is getting better support for building the compiler and the runtime separately (since that's what I rely upon). Fully doing this, however, is somewhat complicated so I'm trying to do it bit by bit.

In this PR I've implemented:

  • The ~= operator in ldc2.conf
  • ldc2.conf being a directory
  • some cmake refactors

The reasoning for this is that both the compiler and the runtime have some sort of switches that they want to encode so, if they are built separately, they can only install (at least) 2 separate files. The ~= operator is mostly a convenience but it is sometimes useful to be able to append a flag without having to copy all of the default ones.

This also has the nice benefit of making the cmake code a little more cleaner since there's no longer a need to go through the whole compiler configuration and then the runtime configuration while keeping track of all the variables that appear in the various ldc2*.conf.in files.

One behavior change that I did do is with:

default: {
	switches = [ "-A" ];
	switches = [ "-B' ];
}

The older code picked -A but now it picks -B. I think this is justified.

I'm curious what you think about this, both the final goal and these specific changes.

@the-horo
Copy link
Contributor Author

It seems that my change to have the multilib filler script depend on ldmd2 instead of ldc2 isn't really correct. On a system without ldc2 installed the build is expected to fail since ldmd2 will probably be built before all the object files of ldc2 and then, when run, it would fail to find a ldc2 executable which will make the whole build fail.

The change was meant to solve the circular dependency issue between every object file that needed to be compiled by the built ldc2 requiring the config file to be configured and the config file requiring ldc2. The old behavior of having everything add a dependency both on ldc2 and the config file I think is the worst approach since it pushes the issue downstream on the whole cmake file instead of solving it at the root.

Other solutions I thought of are:

  • doing some POST_BUILD commands what will generate the files. This is probably the shortest amount of code if we make runtime/CMakeLists.txt just set(MULTILIB_CONF_FILES "..." PARENT_SCOPE) and then generate the full commands in the root CMakeLists.txt and run them. It would be more code if we tried to do the equivalent of set(LDC_POST_BUILD_COMMANDS "..." PARENT_SCOPE) since cmake doesn't have the concept of nested lists but it would look better.
  • slightly refactoring the ldc2 code and putting the triple detection functions in a separate file and just build that single file as a standalone program and use that to figure out the triple.

I'm not sure which one is best. I think I prefer the latter since it feels less hacky but, given that the problem we're trying to solve isn't pretty, the solution that affects the code base the least may be a better choice, even if uglier.

@the-horo
Copy link
Contributor Author

The failure is definitely from here but the logs are not that useful...

@kinke
Copy link
Member

kinke commented Feb 26, 2025

First of all, thx for the work. In order to make this (more easily) reviewable, please split this up, starting with the 1st commit (~=) as a separate PR. That's a nice and most likely uncontroversial change, that I've wanted on multiple occasions myself.

@the-horo the-horo changed the title ldc2.conf changes ldc2.conf: Implement ~= concatenating operator Feb 26, 2025
@kinke
Copy link
Member

kinke commented Feb 26, 2025

Then wrt. the config itself, and making it more easily extensible - my main focus in that regard is simplifying/automating https://wiki.dlang.org/Cross-compiling_with_LDC#Tweaking_the_LDC_configuration_file. I've been promised a little tool (ldc-add-target or so) on multiple occasions in the past, which would download the according prebuilt package from GitHub, extract the libs and copy them to the LDC install dir, and then add a matching ldc2.conf section. Incl. platform specifics, like asking for the cross-gcc/clang executable when targeting a non-Apple Posix platform, edit or the Android NDK location.

Then every user could make use of that tool, to simplify setting up cross-compilation, based on the prebuilt binaries on GitHub.

I'd like to avoid tampering with the config in CMake as much as possible, because it's such a pain, and patching/extending it after a build much simpler. We most likely don't need the 3 *.conf.in files and could reduce them to a single one. And the whole multilib complication is only a thing for keeping up integrated 32-bit x86 support on Linux (and possibly BSDs). Not sure how much longer that's going to be needed, as the 32-bit libs could be created via ldc-build-runtime too (but the 32-bit test coverage would significantly suffer, e.g., no 32-bit dmd-testsuite tests anymore).

@the-horo
Copy link
Contributor Author

Then wrt. the config itself, and making it more easily extensible - my main focus in that regard is simplifying/automating https://wiki.dlang.org/Cross-compiling_with_LDC#Tweaking_the_LDC_configuration_file. I've been promised a little tool (ldc-add-target or so) on multiple occasions in the past, which would download the according prebuilt package from GitHub, extract the libs and copy them to the LDC install dir, and then add a matching ldc2.conf section. Incl. platform specifics, like asking for the cross-gcc/clang executable when targeting a non-Apple Posix platform.

I could at least help with making the package and making the config section (so no cross C toolchain things at first) and I'll start with implementing it on linux first but I could try extending it once I have something working. Being able to do something like -DRUNTIME_TRIPLE=.... and then ninja install being sufficient to get cross-compiling working is something I aspire. However, that's too much work to picture at once so I can only try to get there with small steps so I can't tell if it's possible or how hard it would be, but I do want it to get it in the best state I can.

Then every user could make use of that tool, to simplify setting up cross-compilation, based on the prebuilt binaries on GitHub.

I'd like to avoid tampering with the config in CMake as much as possible, because it's such a pain, and patching/extending it after a build much simpler. We most likely don't need the 3 *.conf.in files and could reduce them to a single one. And the whole multilib complication is only a thing for keeping up integrated 32-bit x86 support on Linux (and possibly BSDs). Not sure how much longer that's going to be needed, as the 32-bit libs could be created via ldc-build-runtime too (but the 32-bit test coverage would significantly suffer, e.g., no 32-bit dmd-testsuite tests anymore).

In regards to multilib, that's exactly what I ended up doing in Gentoo, with only slight adjustments so it's definitely doable.
The testsuite I don't think would be an issue, we can definitely make it a configurable option in which you get what triples you want to test dmd with. That's assuming you even need phobos/druntime. Surely compilable & fail_compilation can work without.

In regards to cmake, making config file generation & configuration more easier was the second goal of my changes. But that's for another PR so we could discuss it there, after I figure out what was failing in CI.

if (b.isAppending)
newVals = [] ~ a.vals ~ b.vals;
else
newVals = [] ~ b.vals;
Copy link
Member

Choose a reason for hiding this comment

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

The [] ~ prefix isn't really needed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You need someway to go from const(string)[] to string[]. .dup or newVals ~= a.vals also works.

Copy link
Member

Choose a reason for hiding this comment

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

I've tried to simplified this, and investigate the macOS x86_64 issue a bit in #4856.


// RUN: %ldc -o- -conf=%S/inputs/invalid_append.conf %s 2>&1 | FileCheck %s --check-prefix=APP
Copy link
Member

@kinke kinke Feb 27, 2025

Choose a reason for hiding this comment

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

So this added command curiously fails for macOS x86_64. I'm wondering if that has anything to do with 2 default sections in the same .conf. Edit: No, seems extremely unlikely after a closer look. Looks more like a segfault during unwinding... we've had similar macOS fun in the past, 'caused' by LTO and/or PGO, and/or different LLVM versions for host clang (Xcode) and LDC (as host compiler for itself).

Similarly to the D counterpart, ~= is used for concatenating arrays.

The new behavior of the config file can be summarized by:
- go through each section (in the order that they appear in the file)
- if the section matches the target triple go through each setting (in
  the order that they appear in the section) and carry out the
  assignments. `=` or `:` override the previous value, `~=` appends to
  it (in the case of arrays).

This change can break some setups, mainly setups in which a section
contained the same key multiple times. For example:
```
default: {
	 switches = [ "A" ]
	 switches = [ "B" ]
}
```

Previously, the above config would pick the flag `A` but this has been
changed to picking the flag `B`. Given that the above is unintuitive
and users should have easily realized that some of their settings were
being ignored, this can be a justified change.

Signed-off-by: Andrei Horodniceanu <a.horodniceanu@proton.me>

Signed-off-by: Andrei Horodniceanu <a.horodniceanu@proton.me>
@MrcSnm
Copy link

MrcSnm commented Mar 2, 2025

I hope you don't intend to keep changing ldc.conf too much.
Currently, on redub, I'm using ldc.conf + --arch to identify which configuration is being used and thus, which linker it is running. That is used mostly because GNU ld has a a problem in which it does not solve symbols as static libraries are shown. But there is a flag that only works for GNU ld which is --start-group and --end-group.

I then need to identify if the user is using that linker or not to add those flags

@the-horo
Copy link
Contributor Author

the-horo commented Mar 2, 2025

I hope you don't intend to keep changing ldc.conf too much. Currently, on redub, I'm using ldc.conf + --arch to identify which configuration is being used and thus, which linker it is running. That is used mostly because GNU ld has a a problem in which it does not solve symbols as static libraries are shown. But there is a flag that only works for GNU ld which is --start-group and --end-group.

I then need to identify if the user is using that linker or not to add those flags

The only other change that I'm pretty sure will happen is the possibility of ldc2.conf becoming a directory (so, on a user system it can be either a file like it is now or it can be a directory). If it's a directory you just go through each file, ordered by name, and reapply the parse algorithm over it.
So, if you really want to do it just change your parseLDCConfig to take the ConfigSection result as a ref param and implement the directory iteration.

But, like I think it has been suggested in the discord channel, isn't it easier to do ldc2 --mtriple=<arch> -L--version - <<<"" and grep for the GNU ld there? I don't know your specific issue but the easiest way I think you can solve it by:

  1. unconditionally passing --start-group and --end-group (if it doesn't break other linkers)
  2. calling ldc2 -L--version --mtriple=<arch> if you know the triple
  3.  echo "/+ dub.sdl: +/" > /tmp/test.d
     DFLAGS=-L--version dub --single /tmp/test.d --arch=<arch>
    
    If you also want to support dub arch shortcuts like x86_64 but don't want to duplicate the code.

Trying to go through ldc2.conf with regexes will just lead to bugs and is way overkill for determining the linker ldc2 is using.

@MrcSnm
Copy link

MrcSnm commented Mar 2, 2025

The only other change that I'm pretty sure will happen is the possibility of ldc2.conf becoming a directory (so, on a user system it can be either a file like it is now or it can be a directory). If it's a directory you just go through each file, ordered by name, and reapply the parse algorithm over it. So, if you really want to do it just change your parseLDCConfig to take the ConfigSection result as a ref param and implement the directory iteration.

But, like I think it has been suggested in the discord channel, isn't it easier to do ldc2 --mtriple=<arch> -L--version - <<<"" and grep for the GNU ld there? I don't know your specific issue but the easiest way I think you can solve it by:

  1. unconditionally passing --start-group and --end-group (if it doesn't break other linkers)

  2. calling ldc2 -L--version --mtriple=<arch> if you know the triple

  3.  echo "/+ dub.sdl: +/" > /tmp/test.d
     DFLAGS=-L--version dub --single /tmp/test.d --arch=<arch>
    

    If you also want to support dub arch shortcuts like x86_64 but don't want to duplicate the code.

Trying to go through ldc2.conf with regexes will just lead to bugs and is way overkill for determining the linker ldc2 is using.

  1. I can't pass --start-group and --end-group unconditionally since I get linker errors saying I sent unknown flags, which is also difficult to deal with
  2. That is actually way slower than checking ldc.conf
  3. Actually I'm not even using regex, I have used a custom algorithm since regex would kill my compilation time and is also slower than that

I would also be grateful for keeping in check the wiki
https://wiki.dlang.org/Using_LDC#:~:text=%EE%80%80LDC%EE%80%81%20looks%20for%20an%20%EE%80%80ldc2.conffile%EE%80%81

Since it helped a lot in identifying possibilities on where it exists

@kinke
Copy link
Member

kinke commented Mar 2, 2025

Using the config for determining the linker used by the invoked C compiler is... adventurous, and totally unreliable.

I also doubt that you really need --{start,end}-group, neither dub nor reggae need it. You normally just need to list the libs in the correct order.

@MrcSnm
Copy link

MrcSnm commented Mar 2, 2025

Neither reggae nor dub can build my engine using GNU ld.
That happens because it is unable to solve those symbols:

Error: undefined reference to `hip.api.renderer.core.HipRendererType hip.hiprenderer.initializer.getRendererTypeFromVersion()`
Error: undefined reference to `hip.api.renderer.core.HipRendererType hip.hiprenderer.initializer.rendererFromString(immutable(char)[])`
Error: undefined reference to `hip.api.renderer.core.IHipRendererImpl hip.hiprenderer.initializer.getRendererWithFallback(hip.api.renderer.core.HipRendererType)`

These symbols aren't solved because of how the dependencies are being specified. initializer needs core, gl also needs core, but since core was already used on the linker command to resolve symbols, it is not specified again.

That behavior only happens on GNU ld and requires either specifying -lrenderer_core again after gl, or using start-end group

@kinke
Copy link
Member

kinke commented Mar 2, 2025

Oh wow, so your project has circular link-time deps. Can't you simply emit the --{start,end}-group with all (Linux/non-Apple Posix?) linkers? It's supported by all 4 of my local linker versions:

$ ld.bfd --help | grep start-group
  -(, --start-group           Start a group
$ ld.gold --help | grep start-group
  -(, --start-group           Start a library search group
$ ld.lld --help | grep start-group
  -(                      Alias for --start-group
  --start-group           Ignored for compatibility with GNU unless you pass --warn-backrefs
$ ld.mold --help | grep start-group
  -(, --start-group           Ignored

@MrcSnm
Copy link

MrcSnm commented Mar 2, 2025

That's really strange, because that necessity of identifying the linker was to solve that issue when lld was linking. Mostly when using ldc2 --link-internally which exists on wasm32/64.

Neither LLD or MSVC requires those start-end group since they already solve that circular linker issue without any additional requirements. The same isn't true for gnu ld.

@kinke
Copy link
Member

kinke commented Mar 2, 2025

Yeah, wasm-ld as wasm lld driver doesn't support that flag. But the ld.lld driver supports it, for gnu compatibility. So you'd need to NOT emit it for wasm targets.

@the-horo
Copy link
Contributor Author

the-horo commented Mar 2, 2025

If these:

ldc2 --version

real	0m0,022s
user	0m0,018s
sys	0m0,004s
echo "module object;" > object.d && ldc2 -L--version object.d

real	0m0,028s
user	0m0,009s
sys	0m0,020s

are too slow for you use case, sure go ahead and try to implement the parsing yourself but you will eventually run into problems if you only implement a subset of the grammar or you don't check all the possible ways someone can specify the linker.

If you still want to go ahead checkout driver/config.d as that contains all the details about what is supported. On your trimmed-down implementation I would advise also supporting section: { instead of just section:\n{ and also making the end-of-value ; optional. These seems like the most likely that you will encounter on user systems.

In your implementation I also spotted some bugs:

  • The default section should always apply, not just when the triple is not specified
  • You're missing the end of section condition. Once you find your first section you keep reading values until EOF, going through all following sections regardless of their triple.
  • You have a trailing ` in your unittest (the first line)

@the-horo
Copy link
Contributor Author

the-horo commented Mar 2, 2025

Yeah, wasm-ld as wasm lld driver doesn't support that flag. But the ld.lld driver supports it, for gnu compatibility. So you'd need to NOT emit it for wasm targets.

Unless ldc2 has been built with LDC_WITH_LLD=OFF, or -link-internally=false

@kinke
Copy link
Member

kinke commented Mar 2, 2025

Unless ldc2 has been built with LDC_WITH_LLD=OFF, or -link-internally=false

No, for wasm in general, as without built-in LLD, the compiler defaults to an external wasm-ld binary. (Which is what -link-internally uses as a library for wasm targets, except that that built-in lld version matches the LLVM version LDC was built against, while the external wasm-ld executable might be a different version.)

@MrcSnm
Copy link

MrcSnm commented Mar 2, 2025

If these:

ldc2 --version

real	0m0,022s
user	0m0,018s
sys	0m0,004s
echo "module object;" > object.d && ldc2 -L--version object.d

real	0m0,028s
user	0m0,009s
sys	0m0,020s

are too slow for you use case, sure go ahead and try to implement the parsing yourself but you will eventually run into problems if you only implement a subset of the grammar or you don't check all the possible ways someone can specify the linker.

If you still want to go ahead checkout driver/config.d as that contains all the details about what is supported. On your trimmed-down implementation I would advise also supporting section: { instead of just section:\n{ and also making the end-of-value ; optional. These seems like the most likely that you will encounter on user systems.

In your implementation I also spotted some bugs:

  • The default section should always apply, not just when the triple is not specified
  • You're missing the end of section condition. Once you find your first section you keep reading values until EOF, going through all following sections regardless of their triple.
  • You have a trailing ` in your unittest (the first line)

You just made me recall that shell execution on linux is way faster than on Windows [I get like 30ms, and my PC is REALLY FAST] , enough to make that time irrelevant. All those timings are way slower on Windows but of course I don't need them on Windows. I might use that -L--version thing on linux.

Also, thanks for the points in my implementation, I didn't knew default were always applied

Yeah, wasm-ld as wasm lld driver doesn't support that flag. But the ld.lld driver supports it, for gnu compatibility. So you'd need to NOT emit it for wasm targets.

So you're saying that I should only avoid start-end group when building for wasm instead?

@kinke
Copy link
Member

kinke commented Mar 2, 2025

So you're saying that I should only avoid start-end group when building for wasm instead?

Exactly. And well, not for Windows obviously, and possibly Apple targets (no idea about Apple's ld64). Recent Android NDKs with lld linkers most likely don't need it, but still support the flag. But Linux and probably BSD linkers should all support this flag.

@MrcSnm
Copy link

MrcSnm commented Mar 2, 2025

Thanks, I'll comment out the ldc conf parser, only avoid those flags for wasm and if that issue arises again, I may consider doing the shell exec approach for linux

@MrcSnm
Copy link

MrcSnm commented Mar 2, 2025

Actually, just got that same issue, but on macOS too:

Linking Error at "hbuild". 
	Redub v1.21.6
	ldc 1.40.0
	Failed with flags: 

Error Program exited with code 1
	'/Users/runner/hostedtoolcache/ldc2/1.40.0/arm64/ldc2-1.40.0-osx-universal/bin/ldc2' -of/Users/runner/.dub/.redub/BC4960[87](https://github.com/MrcSnm/HipremeEngine/actions/runs/13616630113/job/38060541364#step:4:88)38033F82/BC49608738033F82/macos64-hbuild /Users/runner/work/HipremeEngine/HipremeEngine/tools/hbuild/macos64-hbuild.o -g -L--no-as-needed -L--start-group -L/Users/runner/.dub/packages/wasm-sourcemaps/69e6c80/wasm-sourcemaps/libwasm-sourcemaps.a -L/Users/runner/.dub/packages/wasm-reader/0.0.6/wasm-reader/libwasm-reader.a -L/Users/runner/.dub/packages/redub/1.21.6/redub/build/libredub.a -L/Users/runner/.dub/packages/xxhash3/0.0.5/xxhash3/libxxhash3.a -L/Users/runner/.dub/packages/redub/1.21.6/redub/package_suppliers/libpackage_suppliers.a -L/Users/runner/.dub/packages/redub/1.21.6/redub/semver/libsemver.a -L/Users/runner/.dub/packages/redub/1.21.6/redub/dub_sdl_to_json/libdub_sdl_to_json.a -L/Users/runner/.dub/packages/sdlite/1.3.0/sdlite/libsdlite.a -L/Users/runner/.dub/packages/taggedalgebraic/0.11.23/taggedalgebraic/libtaggedalgebraic.a -L/Users/runner/.dub/packages/redub/1.21.6/redub/d_dependencies/libd_dependencies.a -L/Users/runner/.dub/packages/redub/1.21.6/redub/colorize/libcolorize.a -L/Users/runner/.dub/packages/redub/1.21.6/redub/adv_diff/libadv_diff.a -L/Users/runner/.dub/packages/redub/1.21.6/redub/hipjson/libhipjson.a -L/Users/runner/.dub/packages/objc_meta/1.1.0/objc_meta/libobjc_meta.a -L/Users/runner/.dub/packages/my-ip/0.2.0/my-ip/libmy-ip.a -L/Users/runner/.dub/packages/handy-httpd/8.4.5/handy-httpd/libhandy-httpd.a -L/Users/runner/.dub/packages/streams/3.5.0/streams/libstreams.a -L/Users/runner/.dub/packages/slf4d/3.0.1/slf4d/libslf4d.a -L/Users/runner/.dub/packages/path-matcher/1.2.0/path-matcher/libpath-matcher.a -L/Users/runner/.dub/packages/httparsed/1.2.1/httparsed/libhttparsed.a -L/Users/runner/.dub/packages/arsd-official/11.5.3/arsd-official/libarsd-official_terminal.a -L/Users/runner/.dub/packages/arsd-official/11.5.3/arsd-official/libarsd-official_core.a -L/Users/runner/.dub/packages/archive/0.7.1/archive/libarchive.a -L--end-group -L-framework -LFoundation -L-framework -LAppKit -L-lObjC -L-all_load -L-lcurl
		  :
	ld: unknown options: --no-as-needed --start-group --end-group 
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Error: /usr/bin/cc failed with status: 1

Internal Error: HTTP request returned status code 403 ()

So, yeah, I still need to check the default linker since the default ld on mac may cause issues too

@the-horo
Copy link
Contributor Author

the-horo commented Mar 2, 2025

Couldn't you do an OS check and don't pass the flag on windows or mac but pass it on linux or other unixes, like Martin said?

@MrcSnm
Copy link

MrcSnm commented Mar 2, 2025

Couldn't you do an OS check and don't pass the flag on windows or mac but pass it on linux or other unixes, like Martin said?

I do a cached check of default linker in the beginning of the program. I'll check for ld64 only to be on the safe side for now

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

Successfully merging this pull request may close these issues.

4 participants