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 [2] #4856

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kinke
Copy link
Member

@kinke kinke commented Mar 1, 2025

No description provided.

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 <[email protected]>

Signed-off-by: Andrei Horodniceanu <[email protected]>
@kinke kinke force-pushed the ldc2-conf-dir branch 4 times, most recently from d5200d0 to 56a938a Compare March 2, 2025 05:05
@kinke
Copy link
Member Author

kinke commented Mar 2, 2025

After countless CI attempts, it looks like the combination of an older host LDC (v1.35.0 with LLVM 16, or v1.37.0 with LLVM 17 - the macos-13 Xcode is LLVM-16 based), incl. skipping the bootstrap compiler build (as for macOS arm64 already), and disabling mimalloc (apparently causing the dreaded libc++abi: Pure virtual function called!), and disabling LTO (the new config_diag.d test command otherwise still crashed during unwinding with LDC v1.37.0; couldn't get LTO to work with v1.35.0, still some liblto.dylib issue) makes it finally work. Mad.

@kinke kinke force-pushed the ldc2-conf-dir branch 2 times, most recently from f6fa46a to e0c7313 Compare March 2, 2025 13:44
@kinke
Copy link
Member Author

kinke commented Mar 2, 2025

Okay, with LDC v1.35.0 as host/bootstrap compiler, LTO can be restored without breaking config_diag.d. [But had to exclude druntime+Phobos from LTO; they were prebuilt with a macos triple without OS version...]

@the-horo
Copy link
Contributor

the-horo commented Mar 2, 2025

From my little testing it was way easier to make the bug disappear. Either:

  • don't build the D parts with lto
  • don't build the C++ parts with lto
  • don't pass -defaultlib=phobos2-ldc-lto,druntime-ldc-lto
  • use -DCOMPILE_D_MODULES_SEPARATELY=ON
  • use -DLDC_LINK_MANUALLY=ON

I did stop testing once I saw that config_diag.d worked though.

@kinke
Copy link
Member Author

kinke commented Mar 2, 2025

Yeah macOS CI is a bit of a black box, very fragile, and last time we had a similar problem (on arm64 back then), nobody could reproduce all the issues (pure virtual func (mostly sporadic, not deterministic!), unwinding issues in the compiler) locally. Probably as it depends on so many different factors - used Xcode version, the host D compiler (incl. whether it's a universal or native macOS package due to #4857), and now even mimalloc, which was really surprising.

Edit: E.g., the current setup in master requires LTO for the macOS x86_64 build (!). Disabling it makes the built compiler useless (pure virtual func IIRC).

@the-horo
Copy link
Contributor

the-horo commented Mar 2, 2025

I've tried -flto=thin and it seems fine.

If the environment is so poor I would rather drop optimizations and go with a minimalist flag set that doesn't produce broken executables but not like I have any setup to run this on and, from your comment, it seems that not even if you have a device can you actually debug these issues.

@kinke
Copy link
Member Author

kinke commented Mar 2, 2025

I've tried -flto=thin and it seems fine.

Oh wow, that's interesting.

If the environment is so poor I would rather drop optimizations and go with a minimalist flag set that doesn't produce broken executables

So far noone has complained, so I guess they are working and sufficiently (CI-)tested. But yeah, it's definitely not a comfortable feeling. As dropping LTO breaks the build (with mimalloc at least), dropping optimizations wasn't really an option so far. But the C++/D interop LTO is IMO quite important, to inline little one-liners etc., so I'd like to keep that. I don't think PGO was ever a problem.

I'm going to test mimalloc once more; disabling that for both macOS builds (no problems on arm64 so far) is IMO an option on the table, to be on the safer side.

I'm also wondering if there were some breaking macOS-only changes in recent LLVM versions, making mixing bitcode objects of different clang/LDC host compiler LLVM versions problematic.

@the-horo
Copy link
Contributor

the-horo commented Mar 2, 2025

I'm also wondering if there were some breaking macOS-only changes in recent LLVM versions, making mixing bitcode objects of different clang/LDC host compiler LLVM versions problematic.

This is way out of my league in terms of theory but I did test clang-19 once and I got the same issue.

@kinke
Copy link
Member Author

kinke commented Mar 2, 2025

Alright, with LDC v1.35.0 and restored LTO, mimalloc can be restored too without breaking config_diag.d. So matching the host/bootstrap LDC LLVM version with the Xcode LLVM version seems to behave as expected - v1.35.0 with its LLVM 16 is the only version that works with and without (full) LTO, and with/without mimalloc. We already matched the host compiler LLVM versions on macOS arm64 (but can use LDC v1.39.0 with macOS-arm64-specific LLVM 17 there, compatible with the macos-15 Xcode 16).

@kinke kinke force-pushed the ldc2-conf-dir branch 2 times, most recently from 5f8c48a to ae83835 Compare March 3, 2025 18:59
@kinke
Copy link
Member Author

kinke commented Mar 3, 2025

@the-horo: What do you think about the 3rd commit here ('somewhat simplify driver/configfile.d')? I've worked on that before getting an idea of where you wanna go with that config directory. In the case of multiple .conf files and multiple consecutive ConfigFile::readConfig() calls, your original code makes sense, looking up merged ArraySetting instances and checking whether the current file only appends flags without overwriting them. With the existing single config file support, that's not needed though.

@kinke kinke changed the title little test ldc2.conf: Implement ~= concatenating operator [2] Mar 3, 2025
@the-horo
Copy link
Contributor

the-horo commented Mar 3, 2025

It seems alright. On its own the changes look good and I like what you did. Sure, it would require me to do some more changes when (if) the directory option is merged but I don't think they would be so many that your few changes here really affect it. I think discussing about it would take more time then implementing it. Passing the array as a ref parameter to findArraySetting sounds like it would be enough to support it.

When I first wrote my changes I actually wanted to combine the two array merging code paths (the one in readArraySetting and in applyArray) but I didn't come up with it anything. Your changes do what I wanted so no complaining here ;)

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.

2 participants