Skip to content

Define __FILE__ as an empty string in release mode#23196

Closed
aheejin wants to merge 1 commit intoemscripten-core:mainfrom
aheejin:file_blank_release
Closed

Define __FILE__ as an empty string in release mode#23196
aheejin wants to merge 1 commit intoemscripten-core:mainfrom
aheejin:file_blank_release

Conversation

@aheejin
Copy link
Member

@aheejin aheejin commented Dec 17, 2024 •
edited
Loading

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.

@aheejin
Define __FILE__ as an empty string in release mode
ad696a2 
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 aheejin requested review from dschuff and sbc100 December 17, 2024 09:47
@aheejin aheejin mentioned this pull request Dec 17, 2024
Closed
@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

hoodmane commented Dec 17, 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.

@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

hoodmane 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.

@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

hoodmane commented Dec 18, 2024

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

@hoodmane
Copy link
Collaborator

hoodmane commented Dec 18, 2024

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

@aheejin
Copy link
Member Author

aheejin commented Jan 2, 2025

Closing in favor of #23222, #23225, and #23256.

@aheejin aheejin closed this Jan 2, 2025
@aheejin aheejin deleted the file_blank_release branch January 9, 2025 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbc100 sbc100 Awaiting requested review from sbc100

@dschuff dschuff Awaiting requested review from dschuff

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Handling __FILE__ in code size tests in CircleCI

3 participants

@aheejin @hoodmane @sbc100