Page MenuHomePhabricator

[WebAssembly] Address review comments on r352930
Needs ReviewPublic

Authored by sunfish on Mar 18 2019, 4:07 PM.

Details

Reviewers
aaron.ballman
Summary

This patch addresses the review comments on r352930:

  • Removes redundant diagnostic checking code
  • Removes errnoneous use of diag::err_alias_is_definition, which turned out to be ineffective anyway since functions can be defined later in the translation unit and avoid detection.
  • Adds a test for various invalid cases for import_name and import_module.

Diff Detail

Repository
rC Clang

Event Timeline

sunfish created this revision.Mar 18 2019, 4:07 PM

I love functional changes that remove code and add tests -- thank you for these!

Removes errnoneous use of diag::err_alias_is_definition, which turned out to be ineffective anyway since functions can be defined later in the translation unit and avoid detection.

If you need to keep the prohibition that these attributes not be applied to functions with definitions, there are ways to accomplish that (we merge declarations and can decide what to do at that point if we see a declaration that is a definition). Given that, do you still want to lift this restriction?

test/Sema/attr-wasm.c
4

Was this intended to be used somewhere? Probably can just be removed if not.

16

Same here.

Removes errnoneous use of diag::err_alias_is_definition, which turned out to be ineffective anyway since functions can be defined later in the translation unit and avoid detection.

If you need to keep the prohibition that these attributes not be applied to functions with definitions, there are ways to accomplish that (we merge declarations and can decide what to do at that point if we see a declaration that is a definition). Given that, do you still want to lift this restriction?

These attributes don't make sense on definitions. So right now, they are silently ignored when applied to definitions. That's effectively the current behavior too, because the existing check doesn't work as intended, so the change here doesn't change anything in practice yet.

A diagnostic would be slightly tidier, but I'm not familiar enough with clang to do this quickly and didn't want to delay the rest of the patch while I investigated. Do you know the specific places we'd need to change to diagnose this?

sunfish marked an inline comment as done.Mar 19 2019, 5:19 PM
sunfish added inline comments.
test/Sema/attr-wasm.c
4

No, you're right that we don't need this (it's copypasta from another test). I'll remove it from the patch.

Removes errnoneous use of diag::err_alias_is_definition, which turned out to be ineffective anyway since functions can be defined later in the translation unit and avoid detection.

If you need to keep the prohibition that these attributes not be applied to functions with definitions, there are ways to accomplish that (we merge declarations and can decide what to do at that point if we see a declaration that is a definition). Given that, do you still want to lift this restriction?

These attributes don't make sense on definitions. So right now, they are silently ignored when applied to definitions. That's effectively the current behavior too, because the existing check doesn't work as intended, so the change here doesn't change anything in practice yet.

A diagnostic would be slightly tidier, but I'm not familiar enough with clang to do this quickly and didn't want to delay the rest of the patch while I investigated. Do you know the specific places we'd need to change to diagnose this?

I'd be fine addressing it in a follow-up patch, but silently ignoring attributes is something I consider a bug. Users have a *very* difficult time seeing the difference between "accepted and doing its thing" and "silently ignored" depending on the attribute semantics, so that's why we explicitly tell them when we're ignoring an attribute.

As for where to handle this, I'd follow our typical "merge" pattern for attributes. 1) Add Sema::mergeImportNameAttr() that does all your error checking for definitions and creates an attribute if there's no issues, 2) From handleImportNameAttr(), handle your typical semantic checks and then calls S.mergeImportNameAttr() to try to create the attribute, and if it is created, attach it to the declaration, 3) hook the new merge operation up in mergeDeclAttribute() in SemaDecl.cpp.

If you don't address it as part of this patch, can you add a bug to bugzilla so that we get this tracked (if you don't plan to fix the issue soon) and add a test case to this patch demonstrating that we are silently ignoring the attribute with a FIXME comment?

sunfish updated this revision to Diff 197215.Apr 29 2019, 4:15 PM

Implemented proper diagnostics for import_name/import_module on functions with definitions, and updated the test.

I'm unsure of the DIAG_SIZE_SEMA change, but without it, the build fails with this error:

/llvm/tools/clang/lib/Basic/DiagnosticIDs.cpp:72:3: error: static assertion failed: DIAG_SIZE_SEMA is insufficient to contain all diagnostics, it may need to be made larger in DiagnosticIDs.h.
   static_assert(                                                               \
   ^
/llvm/tools/clang/lib/Basic/DiagnosticIDs.cpp:88:1: note: in expansion of macro ‘VALIDATE_DIAG_SIZE’
 VALIDATE_DIAG_SIZE(SEMA)
 ^~~~~~~~~~~~~~~~~~
aaron.ballman added inline comments.May 1 2019, 5:49 AM
include/clang/Basic/DiagnosticIDs.h
39

I think this should be a separate commit, and I'd recommend updating it to 4000 to give ourselves more wiggle room.

include/clang/Basic/DiagnosticSemaKinds.td
9649–9656

How about: "import %select{module|name}0 does not match previous declaration" and "import %select{module|name}0 cannot be applied to a function with a definition"?

include/clang/Sema/Sema.h
2616–2621

I'd rather see the typical pattern used here: one taking a ParsedAttr and the other taking a semantic attribute.

lib/Sema/SemaDeclAttr.cpp
5764–5765

const auto *

5769

I don't see anything testing this note or the preceding warning, can you add some tests that exercise it?

5773

I don't see any tests for this either.

5781–5783

I wonder if we want to generalize this with a template (for the attribute type) if we could generalize the diagnostic text a bit more (or add an additional parameter for it)?

5786–5787

const auto *

5790–5791

Missing tests.

5795

Missing tests.