LTO archive test by AadiWaghray · Pull Request #1050 · wild-linker/wild

AadiWaghray · 2025-08-14T01:31:28Z

PRs #91, #744, and #979 should be sufficient to resolve issue #464. But, I think this test highlights the need for more work on the issue. However, I'm not entirely sure if the test is erroneously written. It gets past the LTO guard, but it doesn't error on an undefined symbol as one would expect. Instead, it errors on "Binary ran for too long."

davidlattimore · 2025-08-14T01:34:13Z

@@ -0,0 +1,5 @@
+//#LinkArgs:-L./ -lexample
+
+extern int CRC32_ProcessSingleBuffer(void const*, int);


Declaring an extern function won't actually cause a reference to the symbol to be emitted. The code needs to actually refer to the symbol - e.g. from main.

davidlattimore · 2025-08-14T01:35:14Z

It's fine to add this file to experimentation, but I think if we wanted to merge this PR, we'd need to figure out how to build a similar file from source.

AadiWaghray · 2025-08-14T01:58:30Z

Apologies for pushing prematurely. There is an issue I need to fix. But, the changes mentioned above have been added.

davidlattimore · 2025-08-14T02:18:30Z

+
+extern int addition(int, int);
+
+int main(int argc, char **argv) {


Most of our tests use _start rather than main. _start is the default entry point for the binary. main is called sometime later by the libc runtime. A few of our tests do use main, but we only do that when necessary, since it requires linking against libc, which substantially slows down linking. I suspect the reason you're seeing a failure is because you haven't defined _start and also haven't linked against libc. I'd suggest looking at a test like trivial.c to see how the _start-based tests are written.

That makes sense. I incorrectly assumed libc was linked automatically for testing since I used main in my z-defs testing. I saw the Requires set of flags but didn't put two and two together. Pushing fixes now, the error is now, correctly, the undefined symbol error showing archives avoid the LTO guard. I still have the issue we discussed earlier of accessing the archive's sections at the point we emit the undefined error to emit an LTO error rather than an undefined error.

davidlattimore · 2025-08-14T03:41:37Z

Looking at one of the test failures:

Error: Failed to build program `lto-undefined.c` with linker `ld` config `default`
  Caused by:
    Linker failed:
/usr/bin/ld.bfd: /__w/wild/wild/wild/tests/build/archive_lto.default-host-2d02e06e5b1d047f.a(archive_lto.default-host-2d02e06e5b1d047f.o): plugin needed to handle lto object
/usr/bin/ld.bfd: /__w/wild/wild/wild/tests/build/archive_lto.default-host-2d02e06e5b1d047f.a(archive_lto.default-host-2d02e06e5b1d047f.o): plugin needed to handle lto object
/usr/bin/ld.bfd: /__w/wild/wild/wild/tests/build/lto-undefined.default-host-c78a7f268027252.o: in function `_start':
lto-undefined.c:(.text+0x24): undefined reference to `addition'

This is failing to link with ld. i.e. it hasn't actually gotten to linking with wild, which happens after GNU ld. GNU ld looks to be failing because the linker plugin wasn't passed as an argument. I guess that suggests that for this test, we do need to use the C compiler to drive the linker rather than invoking the linker directly. If you look at trivial-main.c, you can see how we set up to use GCC as the linker driver - pretty much set //#LinkerDriver:gcc.

AadiWaghray · 2025-08-14T04:05:55Z

The test should be good now!

davidlattimore · 2025-08-14T04:25:54Z

Cool, yep that failure does indeed show that we're still giving an undefined symbol error rather than a more appropriate error. I suspect #979 might have actually regressed that, but I don't think we want to undo that change, since that would hurt performance. The issue is that we now only look for LTO sections in archive entries that we activate, but we never activate the relevant archive entry because it doesn't define any any needed symbols in its symtab.

One possibility would be that if we detect that we've got an undefined symbol error, we go back and scan all archived objects that we didn't already process. If any turn out to have LTO bitcode, then make that be the error instead of the undefined symbol error.

AadiWaghray · 2025-08-14T08:38:02Z

There was something about the tests that was confusing me. If you run cargo test multiple times, then ld will fail every time on the first run. But, every cargo test after that will succeed...As I'm writing this, I realize its because the test assumes the error will be for both linkers. I'll fix that and the formatting in the morning.

fixed: test to build own archive and use function removed: unecessary linker args fixed: lto archive test no longer requires libc link fixed: ld needs lto plugin from gcc to work fixed: lto test to avoid ld error

improved: error message and test fixed: cargo fmt

AadiWaghray · 2025-08-14T17:08:11Z

I'm not quite sure why the tests aren't passing. They pass locally and are giving the error message I expect and catch in the test header.

Edit: I realize what the problem is. I'll push a fix.

fixed: formating

davidlattimore · 2025-08-14T22:04:16Z

+                                .expect("Canonicalized path can't have ../ at end")
+                                .to_str()
+                                .expect("File name should be valid UTF-8")
+                                .split(".")


Why are you only taking the filename up to '.'?

The file generated is archive_lto.a. I believe that the linker adds something to the end of the object file extracted into memory when parsing the archive. I thought it would better to give the name of the actual archive file and avoid the random values. This is the error I mentioned fixing above where the test would pass locally but not in github actions. I never checked where these values are being added onto the name. If we are able to know what it is and add the appropriate value to the Expected header value, it is possible that this is entirely unnecessary.

I suspect the extra stuff in the filename is added by the integration testing framework, so it's actually part of the filename.

After reading integration_tests.rs and reviewing the commits, I see you're 100% right. The command is hashed as part of the file name, and I added the //#SkipLinker:ld directive between commits. I'll change that and everything should be good.

davidlattimore · 2025-08-14T23:26:09Z

+                    .symbol_name(symbol_id)
+                    .expect("Errored on symbol {symbol_name} so it exists");
+                let mut lto_file: Option<&PathBuf> = None;
+                let lto_unresolved = resources.symbol_db.groups.iter().any(|group| match group {


Is lto_unresolved actually needed? It looks like it's always true when lto_file is Some and false when lto_file is None. Perhaps just replace the two any calls with find_map.

That's fair. I was originally just checking if the undefined symbol came from a file with lto, but I then realized I needed to add the file name in the error message.

davidlattimore · 2025-08-14T23:31:12Z

-                if args.error_unresolved_symbols {
+                let raw_symbol_name = symbol_db
+                    .symbol_name(symbol_id)
+                    .expect("Errored on symbol {symbol_name} so it exists");


Note that the {symbol_name} won't substitute the variable symbol_name.

I didn't know that expect doesn't format strings. Thanks for pointing it out!

davidlattimore · 2025-08-14T23:32:28Z

+                let raw_symbol_name = symbol_db
+                    .symbol_name(symbol_id)
+                    .expect("Errored on symbol {symbol_name} so it exists");
+                let mut lto_file: Option<&PathBuf> = None;


This method is already pretty large and adding this makes it ever larger. Could something be moved out to a separate function? Perhaps the code to compute lto_file?

That makes sense. Will work on it.

davidlattimore reviewed Aug 14, 2025

View reviewed changes

AadiWaghray force-pushed the lto-undefined branch from 218c518 to 960e365 Compare August 14, 2025 01:57

AadiWaghray force-pushed the lto-undefined branch from 960e365 to 0a01569 Compare August 14, 2025 02:02

davidlattimore reviewed Aug 14, 2025

View reviewed changes

AadiWaghray force-pushed the lto-undefined branch from 0a01569 to edc398c Compare August 14, 2025 03:30

AadiWaghray force-pushed the lto-undefined branch from edc398c to 30dd1cc Compare August 14, 2025 04:04

AadiWaghray added 2 commits August 14, 2025 09:55

added: lto-undefined test

2846b73

fixed: test to build own archive and use function removed: unecessary linker args fixed: lto archive test no longer requires libc link fixed: ld needs lto plugin from gcc to work fixed: lto test to avoid ld error

added: error message for lto archives

785793a

improved: error message and test fixed: cargo fmt

AadiWaghray force-pushed the lto-undefined branch from 00829c2 to 785793a Compare August 14, 2025 17:03

fixed: panics and file name check in error

629bb4a

fixed: formating

AadiWaghray force-pushed the lto-undefined branch from 1135d6e to 629bb4a Compare August 14, 2025 19:39

AadiWaghray marked this pull request as ready for review August 14, 2025 21:11

davidlattimore reviewed Aug 14, 2025

View reviewed changes

fixed: code organization

51d84e5

AadiWaghray force-pushed the lto-undefined branch from 6e306fa to 51d84e5 Compare August 15, 2025 02:29

fixed: unecessary file name processing

36234de

davidlattimore merged commit 6e709b8 into wild-linker:main Aug 15, 2025
39 of 40 checks passed

AadiWaghray deleted the lto-undefined branch August 15, 2025 17:40

		@@ -0,0 +1,5 @@
		//#LinkArgs:-L./ -lexample

		extern int CRC32_ProcessSingleBuffer(void const*, int);


		extern int addition(int, int);

		int main(int argc, char **argv) {

Uh oh!

Conversation

AadiWaghray commented Aug 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AadiWaghray commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidlattimore commented Aug 14, 2025

Uh oh!

AadiWaghray commented Aug 14, 2025

Uh oh!

davidlattimore commented Aug 14, 2025

Uh oh!

AadiWaghray commented Aug 14, 2025

Uh oh!

AadiWaghray commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AadiWaghray commented Aug 14, 2025 •

edited

Loading

AadiWaghray commented Aug 14, 2025 •

edited

Loading