Page MenuHomePhabricator

[libunwind] Remove __FILE__ and __LINE__ from error reporting
ClosedPublic

Authored by leonardchan on Mar 9 2020, 8:59 PM.

Details

Summary

We were seeing non-deterministic binary size differences depending on which toolchain was used to build fuchsia. This is because libunwind embeded the FILE path into a logging macro, even for release builds, which makes the code dependent on the build directory.

This removes the file and line number from the error message. This is consistent with how other runtimes report error, e.g. https://github.com/llvm/llvm-project/blob/master/libcxxabi/src/abort_message.cpp#L30.

Diff Detail

Event Timeline

leonardchan created this revision.Mar 9 2020, 8:59 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 9 2020, 8:59 PM
phosek added a comment.Mar 9 2020, 9:25 PM

I'd prefer to make this the behavior when just NDEBUG is set, I think it's rather unexpected to still get debug messages even after you set NDEBUG, but since that would be a breaking change, it deserves an email to llvm-dev to ensure that nobody is relying on the current behavior.

leonardchan retitled this revision from [libunwind] Add flag to disable logging to [libunwind] Remove __FILE__ and __LINE__ from error reporting.Mar 10 2020, 12:06 PM
leonardchan edited the summary of this revision. (Show Details)
phosek accepted this revision.Mar 10 2020, 12:12 PM

LGTM but you might want to wait a few more hours for other reviewers to see if anyone has objections.

libunwind/src/config.h
125

I'd consider dropping __func__ as well but I'm fine with keeping it as well.

This revision is now accepted and ready to land.Mar 10 2020, 12:12 PM
saugustine accepted this revision.Mar 10 2020, 12:43 PM

This looks fine to me.

__FILE__ only expands to an absolute path if you pass an absolute path to clang (ctrl-f "FILE" on http://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html).

It feels like this is working around a deficiency in the compiler. Maybe fixing whatever's going wrong in the compiler might be better? (There's also -ffile-file-prefix map, but using that makes your _commandline_ depend on the name of the build dir, and (in clang) __FILE_NAME__ .

__FILE__ only expands to an absolute path if you pass an absolute path to clang (ctrl-f "FILE" on http://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html).

It feels like this is working around a deficiency in the compiler. Maybe fixing whatever's going wrong in the compiler might be better? (There's also -ffile-file-prefix map, but using that makes your _commandline_ depend on the name of the build dir, and (in clang) __FILE_NAME__ .

Using -ffile-prefix-map for LLVM builds in general seems to be the better option, but we thought this was simpler since it seemed to only be libunwind that had this non-determinism.

__FILE__ only expands to an absolute path if you pass an absolute path to clang (ctrl-f "FILE" on http://blog.llvm.org/2019/11/deterministic-builds-with-clang-and-lld.html).

Yes, but unfortunately that's what CMake does by default (that is using absolute paths) so we have to deal with that somehow since CMake is still the official build system.

It feels like this is working around a deficiency in the compiler. Maybe fixing whatever's going wrong in the compiler might be better? (There's also -ffile-file-prefix map, but using that makes your _commandline_ depend on the name of the build dir, and (in clang) __FILE_NAME__ .

This revision was automatically updated to reflect the committed changes.