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

Define __FILE__ as an empty string in release mode #23196

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Dec 17, 2024

This removes all absolute and relative file paths generated by __FILE__ macro from release build, making the release build reproducible and the results of code size tests consistent.

Without -Wno-builtin-macro-redefined, the build errors out:

In file included from <built-in>:365:
<command line>:3:9: error: redefining builtin macro [-Werror,-Wbuiltin-macro-redefined]
    3 | #define __FILE__ ""
      |         ^
1 error generated.

The more detailed description on why this is necessary is in #23195.

Fixes #23195.

This removes all absolute and relative file paths generated by
`__FILE__` macro from release build, making the release build
reproducible and the results of code size tests consistent.

Without `-Wno-builtin-macro-redefined`, the build errors out:
```console
In file included from <built-in>:365:
<command line>:3:9: error: redefining builtin macro [-Werror,-Wbuiltin-macro-redefined]
    3 | #define __FILE__ ""
      |         ^
1 error generated.
```

The more detailed description on why this is necessary is in emscripten-core#23195.
@aheejin
Copy link
Member Author

aheejin commented Dec 17, 2024

This also needs to update the rebaseline results, but somehow running ./tools/maint/rebaseline_tests.py on the main branch on my local machine causes size changes, so I think I should figure that out first.

@hoodmane
Copy link
Collaborator

running ./tools/maint/rebaseline_tests.py on the main branch on my local machine causes size changes

I have the same problem, haven't been able to figure out why.

@aheejin
Copy link
Member Author

aheejin commented Dec 18, 2024

running ./tools/maint/rebaseline_tests.py on the main branch on my local machine causes size changes

I have the same problem, haven't been able to figure out why.

@hoodmane It looks that our LLVM roller was pinned at a specific version to diagnose another potential bug:
https://chromium.googlesource.com/emscripten-releases/+/88e95307accc1f4b247b86b7a0c0e6beaf07f844%5E%21/

So if you are compiling LLVM from source, I think you can set it to 322eb1a92e6d4266184060346616fa0dbe39e731 for the moment to avoid this problem.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 18, 2024

running ./tools/maint/rebaseline_tests.py on the main branch on my local machine causes size changes

I have the same problem, haven't been able to figure out why.

@hoodmane It looks that our LLVM roller was pinned at a specific version to diagnose another potential bug: https://chromium.googlesource.com/emscripten-releases/+/88e95307accc1f4b247b86b7a0c0e6beaf07f844%5E%21/

So if you are compiling LLVM from source, I think you can set it to 322eb1a92e6d4266184060346616fa0dbe39e731 for the moment to avoid this problem.

Or just use emsdk install tot to get the binaries.

@hoodmane
Copy link
Collaborator

I am using emsdk install tot to get the binaries but I still see changes when I rebaseline on the main branch.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 18, 2024

I am using emsdk install tot to get the binaries but I still see changes when I rebaseline on the main branch.

Can you try with --clear-cache?

@sbc100
Copy link
Collaborator

sbc100 commented Dec 18, 2024

If you have installed with emsdk install tot and run ./tools/maint/rebaseline_tests.py --clear-cache and your are still not seeing test expectations are up-to-date then we should investigate why you are seeing discrepencies.

Can you share what revision of main you are on and then pick on the of the test that has a discrepency and then run it on its own and the attach the contents of out/test to this issue ?

@hoodmane
Copy link
Collaborator

Okay I'll try that out on one of the commits that rebaselined code size.

@hoodmane
Copy link
Collaborator

Okay, it seems to be working for me now. Not sure what I was doing wrong before.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 18, 2024

Okay I'll try that out on one of the commits that rebaselined code size.

Your recent change #23135 actually caused main to become out-of-date. So I just landed 91f9825 which updates it again.

At some point we will hopefully make it really easy to update these expectation along with each PR that lands: #23146

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.

Handling __FILE__ in code size tests in CircleCI
3 participants