Re: r293123 - Remove and replace DiagStatePoint tracking and lookup data structure.

2017-01-28 Thread Richard Smith via cfe-commits 
On 28 Jan 2017 8:16 pm, "Duncan P. N. Exon Smith via cfe-commits" <
cfe-commits@lists.llvm.org> wrote:

Hi Richard,

This commit makes one of the tests fail in:
https://reviews.llvm.org/D27689

The implicit modules model expects to be able to "upgrade" an implicit PCM
with more -Werror flags *without* affecting the signature.  However, with
your commit, the command-line diagnostic flags (e.g., -Wconversion vs
-Wno-conversion) now change the AST, and thus change the ASTFileSignature.

One possible hammer would be to disable this pragma-diagnostic-stuff
somehow for implicit modules.  Do you have any other suggestions?


I think that upgrade model is incorrect in the fully general case. For
instance, in C++, enabling more warning flags in the definition of a
template can cause behaviour changes in users of the template. Likewise, in
C, as I recall we'll sometimes look at the warning state at the point of
definition of a macro to determine whether to emit a warning in a macro
expansion.

That said, for implicit modules we can probably assume that the effects of
changing the starting state can be simulated. Right now ,the implicit /
explicit module decision is not made until the point of import, so we would
need to commit to that earlier to avoid emitting the information at all in
that case. Another alternative would be to exclude the diagnostic settings
in the initial state from the hash, either by never emitting it and
recomputing it from the diagnostic flags as needed, or by emitting it into
an unhashed portion of the file.

Thanks,
Duncan

> On 2017-Jan-25, at 17:01, Richard Smith via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
>
> Author: rsmith
> Date: Wed Jan 25 19:01:01 2017
> New Revision: 293123
>
> URL: http://llvm.org/viewvc/llvm-project?rev=293123=rev
> Log:
> Remove and replace DiagStatePoint tracking and lookup data structure.
>
> Rather than storing a single flat list of SourceLocations where the
diagnostic
> state changes (in source order), we now store a separate list for each
FileID
> in which there is a diagnostic state transition. (State for other files is
> built and cached lazily, on demand.) This has two consequences:
>
> 1) We can now sensibly support modules, and properly track the diagnostic
state
> for modular headers (this matters when, for instance, triggering
instantiation
> of a template defined within a module triggers diagnostics).
>
> 2) It's much faster than the old approach, since we can now just do a
binary
> search on the offsets within the FileID rather than needing to call
> isBeforeInTranslationUnit to determine source order (which is surprisingly
> slow). For some pathological (but real world) files, this reduces total
> compilation time by more than 10%.
>
> For now, the diagnostic state points for modules are loaded eagerly. It
seems
> feasible to defer this until diagnostic state information for one of the
> module's files is needed, but that's not part of this patch.
>
> Added:
>cfe/trunk/test/Modules/diag-pragma.cpp
> Modified:
>cfe/trunk/include/clang/Basic/Diagnostic.h
>cfe/trunk/lib/Basic/Diagnostic.cpp
>cfe/trunk/lib/Basic/DiagnosticIDs.cpp
>cfe/trunk/lib/Serialization/ASTReader.cpp
>cfe/trunk/lib/Serialization/ASTWriter.cpp
>cfe/trunk/test/Modules/Inputs/diag_pragma.h
>
> Modified: cfe/trunk/include/clang/Basic/Diagnostic.h
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/
clang/Basic/Diagnostic.h?rev=293123=293122=293123=diff
> 
==
> --- cfe/trunk/include/clang/Basic/Diagnostic.h (original)
> +++ cfe/trunk/include/clang/Basic/Diagnostic.h Wed Jan 25 19:01:01 2017
> @@ -29,6 +29,7 @@
> #include 
> #include 
> #include 
> +#include 
> #include 
> #include 
> #include 
> @@ -232,40 +233,97 @@ private:
>   /// \brief Keeps and automatically disposes all DiagStates that we
create.
>   std::list DiagStates;
>
> -  /// \brief Represents a point in source where the diagnostic state was
> -  /// modified because of a pragma.
> -  ///
> -  /// 'Loc' can be null if the point represents the diagnostic state
> -  /// modifications done through the command-line.
> -  struct DiagStatePoint {
> -DiagState *State;
> -SourceLocation Loc;
> -DiagStatePoint(DiagState *State, SourceLocation Loc)
> -  : State(State), Loc(Loc) { }
> +  /// A mapping from files to the diagnostic states for those files.
Lazily
> +  /// built on demand for files in which the diagnostic state has not
changed.
> +  class DiagStateMap {
> +  public:
> +/// Add an initial diagnostic state.
> +void appendFirst(DiagState *State);
> +/// Add a new latest state point.
> +void append(SourceManager , SourceLocation Loc, DiagState
*State);
> +/// Look up the diagnostic state at a given source location.
> +DiagState *lookup(SourceManager , SourceLocation Loc) const;
> +/// Determine whether this map is empty.
> +bool empty() const { return

Re: r293123 - Remove and replace DiagStatePoint tracking and lookup data structure.

2017-01-28 Thread Duncan P. N. Exon Smith via cfe-commits 
Hi Richard,

This commit makes one of the tests fail in:
https://reviews.llvm.org/D27689

The implicit modules model expects to be able to "upgrade" an implicit PCM with 
more -Werror flags *without* affecting the signature.  However, with your 
commit, the command-line diagnostic flags (e.g., -Wconversion vs 
-Wno-conversion) now change the AST, and thus change the ASTFileSignature.

One possible hammer would be to disable this pragma-diagnostic-stuff somehow 
for implicit modules.  Do you have any other suggestions?

Thanks,
Duncan

> On 2017-Jan-25, at 17:01, Richard Smith via cfe-commits 
>  wrote:
> 
> Author: rsmith
> Date: Wed Jan 25 19:01:01 2017
> New Revision: 293123
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=293123=rev
> Log:
> Remove and replace DiagStatePoint tracking and lookup data structure.
> 
> Rather than storing a single flat list of SourceLocations where the diagnostic
> state changes (in source order), we now store a separate list for each FileID
> in which there is a diagnostic state transition. (State for other files is
> built and cached lazily, on demand.) This has two consequences:
> 
> 1) We can now sensibly support modules, and properly track the diagnostic 
> state
> for modular headers (this matters when, for instance, triggering instantiation
> of a template defined within a module triggers diagnostics).
> 
> 2) It's much faster than the old approach, since we can now just do a binary
> search on the offsets within the FileID rather than needing to call
> isBeforeInTranslationUnit to determine source order (which is surprisingly
> slow). For some pathological (but real world) files, this reduces total
> compilation time by more than 10%.
> 
> For now, the diagnostic state points for modules are loaded eagerly. It seems
> feasible to defer this until diagnostic state information for one of the
> module's files is needed, but that's not part of this patch.
> 
> Added:
>cfe/trunk/test/Modules/diag-pragma.cpp
> Modified:
>cfe/trunk/include/clang/Basic/Diagnostic.h
>cfe/trunk/lib/Basic/Diagnostic.cpp
>cfe/trunk/lib/Basic/DiagnosticIDs.cpp
>cfe/trunk/lib/Serialization/ASTReader.cpp
>cfe/trunk/lib/Serialization/ASTWriter.cpp
>cfe/trunk/test/Modules/Inputs/diag_pragma.h
> 
> Modified: cfe/trunk/include/clang/Basic/Diagnostic.h
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Diagnostic.h?rev=293123=293122=293123=diff
> ==
> --- cfe/trunk/include/clang/Basic/Diagnostic.h (original)
> +++ cfe/trunk/include/clang/Basic/Diagnostic.h Wed Jan 25 19:01:01 2017
> @@ -29,6 +29,7 @@
> #include 
> #include 
> #include 
> +#include 
> #include 
> #include 
> #include 
> @@ -232,40 +233,97 @@ private:
>   /// \brief Keeps and automatically disposes all DiagStates that we create.
>   std::list DiagStates;
> 
> -  /// \brief Represents a point in source where the diagnostic state was
> -  /// modified because of a pragma.
> -  ///
> -  /// 'Loc' can be null if the point represents the diagnostic state
> -  /// modifications done through the command-line.
> -  struct DiagStatePoint {
> -DiagState *State;
> -SourceLocation Loc;
> -DiagStatePoint(DiagState *State, SourceLocation Loc)
> -  : State(State), Loc(Loc) { } 
> +  /// A mapping from files to the diagnostic states for those files. Lazily
> +  /// built on demand for files in which the diagnostic state has not 
> changed.
> +  class DiagStateMap {
> +  public:
> +/// Add an initial diagnostic state.
> +void appendFirst(DiagState *State);
> +/// Add a new latest state point.
> +void append(SourceManager , SourceLocation Loc, DiagState *State);
> +/// Look up the diagnostic state at a given source location.
> +DiagState *lookup(SourceManager , SourceLocation Loc) const;
> +/// Determine whether this map is empty.
> +bool empty() const { return Files.empty(); }
> +/// Clear out this map.
> +void clear() {
> +  Files.clear();
> +  FirstDiagState = CurDiagState = nullptr;
> +  CurDiagStateLoc = SourceLocation();
> +}
> +
> +/// Grab the most-recently-added state point.
> +DiagState *getCurDiagState() const { return CurDiagState; }
> +/// Get the location at which a diagnostic state was last added.
> +SourceLocation getCurDiagStateLoc() const { return CurDiagStateLoc; }
> +
> +  private:
> +/// \brief Represents a point in source where the diagnostic state was
> +/// modified because of a pragma.
> +///
> +/// 'Loc' can be null if the point represents the diagnostic state
> +/// modifications done through the command-line.
> +struct DiagStatePoint {
> +  DiagState *State;
> +  unsigned Offset;
> +  DiagStatePoint(DiagState *State, unsigned Offset)
> +: State(State), Offset(Offset) { } 
> +};
> +
> +/// Description of the diagnostic states and state

r293123 - Remove and replace DiagStatePoint tracking and lookup data structure.

2017-01-25 Thread Richard Smith via cfe-commits 
Author: rsmith
Date: Wed Jan 25 19:01:01 2017
New Revision: 293123

URL: http://llvm.org/viewvc/llvm-project?rev=293123=rev
Log:
Remove and replace DiagStatePoint tracking and lookup data structure.

Rather than storing a single flat list of SourceLocations where the diagnostic
state changes (in source order), we now store a separate list for each FileID
in which there is a diagnostic state transition. (State for other files is
built and cached lazily, on demand.) This has two consequences:

1) We can now sensibly support modules, and properly track the diagnostic state
for modular headers (this matters when, for instance, triggering instantiation
of a template defined within a module triggers diagnostics).

2) It's much faster than the old approach, since we can now just do a binary
search on the offsets within the FileID rather than needing to call
isBeforeInTranslationUnit to determine source order (which is surprisingly
slow). For some pathological (but real world) files, this reduces total
compilation time by more than 10%.

For now, the diagnostic state points for modules are loaded eagerly. It seems
feasible to defer this until diagnostic state information for one of the
module's files is needed, but that's not part of this patch.

Added:
cfe/trunk/test/Modules/diag-pragma.cpp
Modified:
cfe/trunk/include/clang/Basic/Diagnostic.h
cfe/trunk/lib/Basic/Diagnostic.cpp
cfe/trunk/lib/Basic/DiagnosticIDs.cpp
cfe/trunk/lib/Serialization/ASTReader.cpp
cfe/trunk/lib/Serialization/ASTWriter.cpp
cfe/trunk/test/Modules/Inputs/diag_pragma.h

Modified: cfe/trunk/include/clang/Basic/Diagnostic.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Diagnostic.h?rev=293123=293122=293123=diff
==
--- cfe/trunk/include/clang/Basic/Diagnostic.h (original)
+++ cfe/trunk/include/clang/Basic/Diagnostic.h Wed Jan 25 19:01:01 2017
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -232,40 +233,97 @@ private:
   /// \brief Keeps and automatically disposes all DiagStates that we create.
   std::list DiagStates;
 
-  /// \brief Represents a point in source where the diagnostic state was
-  /// modified because of a pragma.
-  ///
-  /// 'Loc' can be null if the point represents the diagnostic state
-  /// modifications done through the command-line.
-  struct DiagStatePoint {
-DiagState *State;
-SourceLocation Loc;
-DiagStatePoint(DiagState *State, SourceLocation Loc)
-  : State(State), Loc(Loc) { } 
+  /// A mapping from files to the diagnostic states for those files. Lazily
+  /// built on demand for files in which the diagnostic state has not changed.
+  class DiagStateMap {
+  public:
+/// Add an initial diagnostic state.
+void appendFirst(DiagState *State);
+/// Add a new latest state point.
+void append(SourceManager , SourceLocation Loc, DiagState *State);
+/// Look up the diagnostic state at a given source location.
+DiagState *lookup(SourceManager , SourceLocation Loc) const;
+/// Determine whether this map is empty.
+bool empty() const { return Files.empty(); }
+/// Clear out this map.
+void clear() {
+  Files.clear();
+  FirstDiagState = CurDiagState = nullptr;
+  CurDiagStateLoc = SourceLocation();
+}
+
+/// Grab the most-recently-added state point.
+DiagState *getCurDiagState() const { return CurDiagState; }
+/// Get the location at which a diagnostic state was last added.
+SourceLocation getCurDiagStateLoc() const { return CurDiagStateLoc; }
+
+  private:
+/// \brief Represents a point in source where the diagnostic state was
+/// modified because of a pragma.
+///
+/// 'Loc' can be null if the point represents the diagnostic state
+/// modifications done through the command-line.
+struct DiagStatePoint {
+  DiagState *State;
+  unsigned Offset;
+  DiagStatePoint(DiagState *State, unsigned Offset)
+: State(State), Offset(Offset) { } 
+};
+
+/// Description of the diagnostic states and state transitions for a
+/// particular FileID.
+struct File {
+  /// The diagnostic state for the parent file. This is strictly redundant,
+  /// as looking up the DecomposedIncludedLoc for the FileID in the Files
+  /// map would give us this, but we cache it here for performance.
+  File *Parent = nullptr;
+  /// The offset of this file within its parent.
+  unsigned ParentOffset = 0;
+  /// Whether this file has any local (not imported from an AST file)
+  /// diagnostic state transitions.
+  bool HasLocalTransitions = false;
+  /// The points within the file where the state changes. There will always
+  /// be at least one of these (the state on entry to the file).
+  llvm::SmallVector StateTransitions;
+
+  DiagState *lookup(unsigned Offset)