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

Druntime test merge fix #4883

Open
wants to merge 9 commits into
base: merge-2.111
Choose a base branch
from

Conversation

the-horo
Copy link
Contributor

Fix the druntime tests merge conflicts as well as resolve some issues with the test code. Some changes are specific to LDC and should remain here, some are bugs that should be fixed upstream but currently they're all in separate commits and will be split and squashed later.

run: |
brew install make
echo "PATH=/usr/local/opt/make/libexec/gnubin:$PATH" >> $GITHUB_ENV
make --version
Copy link
Member

Choose a reason for hiding this comment

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

Won't work on the M1 runners (at least last time I checked): https://github.com/dlang/dmd/pull/17035/files#r1997643588

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was sure that that PATH was going to break eventually. I suppose we don't need the PATH modifications since we're checking for gmake first in

find_program(GNU_MAKE_BIN NAMES gmake gnumake make)

Copy link
Member

Choose a reason for hiding this comment

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

Right, the macOS arm64 job seems happy as-is, so we can remove it. 👍

@@ -1,5 +1,5 @@
# set explicitly in the make cmdline in druntime/Makefile (`test/%/.run` rule):
ifneq (,$(findstring ldmd2,$(DMD)))
ifdef IN_LDC
Copy link
Member

Choose a reason for hiding this comment

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

Mm, much better, I like! 👍

@@ -82,12 +76,9 @@ foreach(name ${testnames})
ROOT=${outdir} DMD=${LDMD_EXE_FULL} BUILD=${build} SHARED=1
CC=${CMAKE_C_COMPILER} CXX=${CMAKE_CXX_COMPILER}
DRUNTIME=${druntime_path_build} DRUNTIMESO=${shared_druntime_path_build}
CFLAGS_BASE=${cflags_base} DFLAGS_BASE=${dflags_base} ${linkdl}
CFLAGS=${cflags_base} CXXFLAGS=${cflags_base} DFLAGS=${dflags_base} ${linkdl}
Copy link
Member

@kinke kinke Mar 18, 2025

Choose a reason for hiding this comment

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

I wonder if we can really override DFLAGS with the (optional) base flags - don't we lose the implicit [-g -debug | -O -release] -w as default DFLAGS this way?

Edit: Oh well, and the same for CFLAGS and CXXFLAGS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intended behavior. If one of those flags (notably -g) is required by the tests then that should be specified in the makefile, outside of DFLAGS. If we want to test multiple configurations (like LINK_SHARED=1 or DFLAGS=-m32 or even DFLAGS="-O -release") then these flags should be specified explicitly.

If we want to keep testing both sets of flags we could reintroduce the DFLAGS_BASE variables to preserve the defaults from the makefile or we could pass D_EXTRA_FLAGS in DMD which would be appropriate if that cmake variable is meant for -m32 or -mtriple like arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can also start passing LDFLAGS=${LD_FLAGS} but, if those flags are only meant for C sources, we would also need LDFLAGS.d="" since common.mak autofills the latter based on the former. There's also d_platform_libs which is used by stdcpp, maybe we could set that to C_SYSTEM_LIBS?

@@ -186,9 +157,17 @@ extra_ldlibs.d += -L$(druntime_for_linking)

extra_ldflags.d += -defaultlib=
extra_ldlibs.d += $(d_platform_libs)
else # IN_LDC
extra_ldflags.d += -defaultlib=druntime-ldc
extra_ldflags.d += $(if $(LINK_SHARED),-link-defaultlib-shared)
Copy link
Member

Choose a reason for hiding this comment

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

-link-defaultlib-shared isn't a pure link-only flag, but also affects some compile flag defaults for Windows target (e.g., defaulting to -dllimport=defaultLibsOnly when not using -fvisibility...). So needs to be specified when compiling too (extra_dflags).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dmd branch adds -dllimport to ldflags, should that be changed as well?

@@ -0,0 +1,3 @@
// ldc fails to link a file named invariant.d
// https://github.com/ldc-developers/ldc/issues/4879
module ldc_rename_me;
Copy link
Member

Choose a reason for hiding this comment

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

This was fixed upstream and already landed in merge-2.111.

[Note that you don't need to rebase unless there are merge conflicts; each push to your branch triggers a new GHA workflow for a freshly merged head against the target branch. Dummy-amending the last commit (without any changes) and force-pushing is a way to re-trigger CI against the latest target branch.]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I usually chose to rebase since I have do it eventually and it doesn't take me that long to drop a commit.

# LDC: required for executables to implicitly dllimport DLL data symbols
DFLAGS+=-dllimport=all
extra_ldflags.d += -dllimport=all
Copy link
Member

Choose a reason for hiding this comment

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

This is pure compile flag only, doesn't affect linking at all.

Copy link
Member

Choose a reason for hiding this comment

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

Btw for DMD this is done in common.mak:153. I guess it doesn't matter where it's defined, this shared/Makefile is IIRC the only one setting LINK_SHARED.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you remember I do want to also start testing with the shared druntime when building both static&dynamic runtime libs so I want to support that flag being passed on the CLI so I'd rather have the logic for it in common.mak

$(ROOT)/dllgc$(DOTEXE): $(SRC)/dllgc.d
$(QUIET)$(DMD) $(DFLAGS) -version=DLL -shared -of$(ROOT)/dllgc$(DOTDLL) $<
$(QUIET)$(DMD) $(DFLAGS) -of$@ $<
$(ROOT)/dllgc$(DOTEXE): private extra_ldflags.d += -link-defaultlib-shared=false -dllimport=none
Copy link
Member

Choose a reason for hiding this comment

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

required for compilation too, not just linking

endif
$(OBJDIR)/dll_gc_proxy_teardown%$(DOTDLL): private extra_ldflags.d += -L/EXPORT:gc_setProxy -L/EXPORT:gc_clrProxy
$(OBJDIR)/dll_gc_proxy_teardown%$(DOTDLL): private extra_dflags += -version=DLL
$(OBJDIR)/dll_gc_proxy_teardown%$(DOTEXE): private extra_ldflags.d += -link-defaultlib-shared=false -dllimport=none
Copy link
Member

Choose a reason for hiding this comment

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

dflags/ldflags need tweaking again

@kinke
Copy link
Member

kinke commented Mar 18, 2025

It might be worth reconsidering splitting the D flags into compile- and link-flags - there's no harm in including link-flags in compiles too, just a bit longer/more explicit command lines in the make output (normally hidden).

@the-horo
Copy link
Contributor Author

It might be worth reconsidering splitting the D flags into compile- and link-flags - there's no harm in including link-flags in compiles too, just a bit longer/more explicit command lines in the make output (normally hidden).

DFLAGS and extra_dflags do apply to both compilation and linking, on top of most of the tests doing compilation+linking in one compiler invocation (stdcpp is the only one that does manual compilation because of the need to link the C++ stdlib).

I would rather not change this design because I purposefully made it follow the way other build systems do it, and if they do it this way I assume that there is a reason for it. So, until I fully understand this reason I would rather go with the possibly sub-optimal design.

@the-horo the-horo force-pushed the pr-2.111 branch 2 times, most recently from 1f5a162 to d2ca7c6 Compare March 18, 2025 08:03
Signed-off-by: Andrei Horodniceanu <a.horodniceanu@proton.me>
The $(OBJDIR) variable already contains the ./ leading prefix and,
when it is set to an absolute path (like ldc does), prepending ./ is
an error.

Signed-off-by: Andrei Horodniceanu <a.horodniceanu@proton.me>
The druntime testsuite requires a more recent version of make then
what is shipped by default with macos.

Signed-off-by: Andrei Horodniceanu <a.horodniceanu@proton.me>
As per msvc docs, when a directory is specified to /Fo it should end
with a slash. cl.exe seems to handle the argument anyway but
clang-cl.exe fails with it, which is used by ldc.

Signed-off-by: Andrei Horodniceanu <a.horodniceanu@proton.me>
When linking executables through CC also pass -rpath to the directory
that contains the druntime library, in case the shared variant would
be linked.

This is currently only a problem for ldc because the posix linkers
encode the full path to the druntime library in DT_NEEDED (which
avoids requiring -rpath during runtime) only if said library has no
soname. ldc's druntime is built with soname but dmd's isn't which hid
the issue.

Signed-off-by: Andrei Horodniceanu <a.horodniceanu@proton.me>
This just discarded the -mMODEL flag on windows which is obviously
undesired.

Signed-off-by: Andrei Horodniceanu <a.horodniceanu@proton.me>
The -rpath argument is correctly handled by the tests which need it.

Signed-off-by: Andrei Horodniceanu <a.horodniceanu@proton.me>
The makefile rewrite upstream no longer defaults the value of CXXFLAGS
to CFLAGS.

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

Since it's a flag that affects compilation for correctness it should be
passed in dflags rather than ldflags.d. There were no issues visible
because the compilation and linking are done in one step in most of
the tests.

Signed-off-by: Andrei Horodniceanu <a.horodniceanu@proton.me>
@the-horo
Copy link
Contributor Author

I've marked which commits should be upstreamed.

The failure which I gathered are:

  • windows druntime-test-exceptions-debug > winstack
  • windows druntime-test-shared > loadDR
  • alpine druntime-test-shared > loadDR
  • macos druntime-test-shared > load_linkdep
  • most platforms importc_compare

The loadDR test on windows logs:

C:/Program Files/LLVM/bin/clang-cl.exe  /FoD:/a/ldc/build/runtime/druntime-test-shared-debug/       src/loadDR.c    /FeD:/a/ldc/build/runtime/druntime-test-shared-debug/loadDR.exe
Testing loadDR
D:/a/ldc/build/runtime/druntime-test-shared-debug/loadDR.exe D:/a/ldc/build/lib/druntime-ldc-debug-shared.lib
Assertion failed: h != NULL, file src/loadDR.c, line 11
  • D:/a/ldc/build/runtime/druntime-test-shared-debug/loadDR.exe D:/a/ldc/build/lib/druntime-ldc-debug-shared.lib
    

Shouldn't the library path on windows by the .dll instead of the .lib?

@the-horo
Copy link
Contributor Author

Yes! The old makefile changed the suffix from .lib to .dll:

$(ROOT)/loadDR.done $(ROOT)/host.done: RUN_ARGS:=$(DRUNTIMESO:.lib=.dll)

But I changed that as I assumed the DRUNTIMESO would be the dll:
$(ROOT)/loadDR.done: private run_args = $(DRUNTIMESO)

which it is within dmd but not within ldc.

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