This is an archive of the discontinued LLVM Phabricator instance.

Differential D81688

[WebAssembly] WebAssembly doesn't support "protected" visibility
ClosedPublic

Authored by sunfish on Jun 11 2020, 1:45 PM.

Details

Reviewers
sbc100
Summary

mplement the hasProtectedVisibility() hook to indicate that, like
Darwin, WebAssembly doesn't support "protected" visibility.

On ELF, "protected" visibility is intended to be an optimization, however
in practice it often [isn't], and ELF documentation generally ranges from
[not mentioning it at all] to [strongly discouraging its use].

[isn't]: https://www.airs.com/blog/archives/307
[not mentioning it at all]: https://gcc.gnu.org/wiki/Visibility
[strongly discouraging its use]: https://www.akkadia.org/drepper/dsohowto.pdf

While here, also mention the new Reactor support in the release notes.

Diff Detail

Event Timeline

sunfish created this revision.Jun 11 2020, 1:45 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 11 2020, 1:45 PM
Herald added subscribers: llvm-commits, aheejin, jgravelle-google and 2 others. · View Herald Transcript
Harbormaster completed remote builds in B60029: Diff 270223.Jun 11 2020, 3:27 PM
sbc100 accepted this revision.Jun 11 2020, 11:18 PM
sbc100 added inline comments.
clang/lib/Basic/Targets/WebAssembly.h
139

I'm not sure we need this compatibility. For sure we don't implement any special behaviour based on this value I'm not sure if we want to warn either.

For now I guess this a conservative choice, but we we should try to replace with with simply return false in the future.

Perhaps you could update the comment to be more like a TODO? lgtm either way.

This revision is now accepted and ready to land.Jun 11 2020, 11:18 PM
sunfish closed this revision.Jun 12 2020, 7:53 PM

Landed in 66042959590d6db9d2a12803a16476d4e3508f3f.

Revision Contents

PathSize
clang/
lib/
Basic/
Targets/
7 lines
llvm/
docs/
10 lines

Diff 270223

clang/lib/Basic/Targets/WebAssembly.h

Context not available.
} }
bool hasExtIntType() const override { return true; } bool hasExtIntType() const override { return true; }
bool hasProtectedVisibility() const override {
// For compatibility, continue to advertise "protected" support for
// Emscripten targets.
return getTriple().isOSEmscripten();
sbc100Unsubmitted
Not Done
ReplyInline Actions

I'm not sure we need this compatibility. For sure we don't implement any special behaviour based on this value I'm not sure if we want to warn either.

For now I guess this a conservative choice, but we we should try to replace with with simply return false in the future.

Perhaps you could update the comment to be more like a TODO? lgtm either way.

sbc100: I'm not sure we need this compatibility. For sure we don't implement any special behaviour…
}
}; };
class LLVM_LIBRARY_VISIBILITY WebAssembly32TargetInfo class LLVM_LIBRARY_VISIBILITY WebAssembly32TargetInfo
: public WebAssemblyTargetInfo { : public WebAssemblyTargetInfo {
public: public:
Context not available.

llvm/docs/ReleaseNotes.rst

Context not available.
Changes to the WebAssembly Target Changes to the WebAssembly Target
--------------------------------- ---------------------------------
During this release ... * Programs which don't have a "main" function, called "reactors" are now
properly supported, with a new `-mexec-model=reactor` flag. Programs which
previously used `-Wl,--no-entry` to avoid having a main function should
switch to this new flag, so that static initialization is properly
performed.
* `__attribute__((visibility("protected")))` now evokes a warning, as
WebAssembly does not support "protected" visibility.
Changes to the OCaml bindings Changes to the OCaml bindings
----------------------------- -----------------------------
Context not available.