[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-commits
Subject:    [PATCH] D58743: Handle built-in when importing SourceLocation and FileID
From:       Shafik Yaghmour via Phabricator via cfe-commits <cfe-commits () lists ! llvm ! org>
Date:       2019-02-28 20:25:41
Message-ID: 033f7384eb7ac977f6ba53d1e7a6c5cb () localhost ! localdomain
[Download RAW message or body]

shafik added a comment.

In D58743#1413249 <https://reviews.llvm.org/D58743#1413249>, @lebedev.ri wrote:

> Is there a test missing here?


This origin of this fix is the LLDB expression parsing issue. We were not able to \
reduce to a test we could put in `ASTImpoterTest.cpp` but we have a LLDB test that \
exercises this issue and will pass once this fix is in place.

See this differential https://reviews.llvm.org/D58790



================
Comment at: lib/AST/ASTImporter.cpp:8284
+    
+    if (!isBuiltin) {
+       // Include location of this file.
----------------
balazske wrote:
> The `Cache` can be moved into this block and the block to `else if`.
I am not sure I understand what you are asking. Do you mean just duplicate the 

    const SrcMgr::ContentCache *Cache = FromSLoc.getFile().getContentCache();

in both blocks?


================
Comment at: lib/AST/ASTImporter.cpp:8301
+            ToID = ToSM.createFileID(Entry, ToIncludeLoc,
+                                     FromSLoc.getFile().getFileCharacteristic());
+        }
----------------
balazske wrote:
> Is it possible that in the `isBuiltin` true case this part of the code does run \
> (always) without assertion or other error and the result is always an invalid \
> `ToID`? (If yes the whole change is not needed, so I want to know what was the \
> reason for this change, was there any crash or bug.)
This change was the result of a assert where evaluating an expression resulted in the \
built-in being imported and it would assert in `SourceManager` when when \
`Import(SourceLocation....)`   attempted `getDecomposedLoc`  at the next step.

AFAICT the The Preprocessor sets it up the buffer \
[here](https://github.com/llvm-mirror/clang/blob/master/lib/Lex/Preprocessor.cpp#L552) \
and then creates the `FileID` for in the `SourceManager` and copying over the buffer \
should be the correct thing to do since that seems to be sufficient. 

I also while testing verified that the `BufferIndetifer()` did indeed equal \
`"<built-in>"` similar to [this \
check](https://github.com/llvm-mirror/clang/blob/master/lib/Basic/SourceManager.cpp#L2009) \
in `SourceManager`.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58743/new/

https://reviews.llvm.org/D58743



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[prev in list] [next in list] [prev in thread] [next in thread]