This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Work with multiple pragmas weak before definition
ClosedPublic

Authored by hubert.reinterpretcast on Mar 17 2022, 10:22 AM.

Details

Summary

Update WeakUndeclaredIdentifiers to hold a collection of weak aliases per identifier instead of only one.

This also allows the "used" state to be removed from WeakInfo because it is really only there as an alternative to removing processed map entries, and we can represent that using an empty set now. The serialization code is updated for the removal of the field. Additionally, a PCH test is added for the new functionality.

The records are grouped by the "target" identifier, which was already being used as a key for lookup purposes. We also store only one record per alias name; combined, this means that diagnostics are grouped by the "target" and limited to one per alias (which should be acceptable).

Fixes PR28611.
Fixes llvm/llvm-project#28985.

Co-authored-by: Rachel Craik <rcraik@ca.ibm.com>
Co-authored-by: Jamie Schmeiser <schmeise@ca.ibm.com>

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2022, 10:22 AM
hubert.reinterpretcast requested review of this revision.Mar 17 2022, 10:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2022, 10:22 AM
cebowleratibm accepted this revision.Mar 18 2022, 12:43 PM

Makes sense to me.

clang/include/clang/Sema/Weak.h
34–51

For my selfish education: what is the purpose of marking this deleted rather than actually deleting it?

clang/test/CodeGen/pragma-weak.c
144

small suggestion for test: repeat this line so that you cover the case where you're collecting identical WeakInfos to the same AliasName.

This revision is now accepted and ready to land.Mar 18 2022, 12:43 PM
hubert.reinterpretcast marked an inline comment as done.Mar 18 2022, 1:02 PM
hubert.reinterpretcast added inline comments.
clang/include/clang/Sema/Weak.h
34–51

Removing this should be fine (I will make the change). Didn't want to delete the operator functions (immediately) though: downstream consumers might end up calling a different function, which warrants some scrutiny.

hubert.reinterpretcast marked an inline comment as done.
  • Address review comments: Remove deleted non-operator member functions; add extra line to test
hubert.reinterpretcast marked an inline comment as done.Mar 21 2022, 11:51 AM

Can you also add a release note for the fix?

clang/include/clang/Sema/Sema.h
1074–1075

Should probably update this comment as the type just got much weirder than before.

clang/include/clang/Sema/Weak.h
19–20
25–29
30

Would it be onerous to make the return type be const IdentifierInfo * given that the function is const? (If it is, just ignore the suggestion -- I love adding const correctness where we can get it basically for free.)

61

Previously we cared about the source locations being the same. But... is there a situation where the aliases are the same but the source locations are different? I can't think of any, so perhaps that's worth an assert that the locations match?

hubert.reinterpretcast marked an inline comment as done.Mar 22 2022, 12:38 PM
hubert.reinterpretcast added inline comments.
clang/include/clang/Sema/Weak.h
61

We didn't really case about the source locations being the same. The removed operators were just dead code (featuring such things as compare-by-pointer-value). I have verified that splitting the deletion of them to a separate patch does not cause any build or LIT check-all failures. I can commit that part as an NFC patch first if you prefer.

Also, the aliases can be the same with different source locations. The location comes from where the #pragma weak is (from the parser by way of ActOnPragmaWeakAlias). That duplicates are ignored (meaning we only diagnose one instance) is mentioned in the patch description.

It is already the status quo that having the same alias specified with different targets (at least those not yet declared) neither produces a diagnostic nor operates by "last one wins". Instead, Clang prefers the first declared target and GCC just emits both and leaves it to the assembler to figure out (https://godbolt.org/z/EasK375j3).

hubert.reinterpretcast marked an inline comment as done.Mar 22 2022, 1:10 PM
hubert.reinterpretcast added inline comments.
clang/include/clang/Sema/Weak.h
30

It's not free but I can post an NFC patch to add the const through. On the chain of calls that const needs to be added to, the pointer eventually feeds a DeclarationName (which already takes const IdentifierInfo *).

hubert.reinterpretcast marked 2 inline comments as done.
  • Address review comments: Use default member init in WeakInfo
clang/include/clang/Sema/Weak.h
30
  • Address review comments: Add release notes, expand comments
hubert.reinterpretcast marked an inline comment as done.Mar 22 2022, 5:02 PM
hubert.reinterpretcast marked an inline comment as done.
  • Address review comments: Return const from getAlias()

@aaron.ballman, I believe I have responded to all of the comments.

aaron.ballman accepted this revision.Mar 23 2022, 9:47 AM

LGTM, thank you for the fix!

clang/include/clang/Sema/Weak.h
61

Ah, thank you for the explanation, that makes sense to me.

clang/include/clang/Sema/Weak.h
47

The fact that WeakUndeclaredIdentifiers is keyed by IdentifierInfo * must mean that it is safe to use pointer comparison. This is further bolstered by IdentifierInfo having only private or deleted constructors (with IdentifierTable being a friend class).

I will adjust to make this class simply forward to DenseMapInfo<const IdentifierInfo *> with the alias members.

aaron.ballman added inline comments.Mar 23 2022, 11:38 AM
clang/include/clang/Sema/Weak.h
47

SGTM!

  • Adjust per observation: Use DenseMapInfo for the alias pointer value
hubert.reinterpretcast marked 2 inline comments as done.Mar 23 2022, 2:54 PM