Re: r348154 - Avoid emitting redundant or unusable directories in DIFile metadata entries.

2018-12-04 Thread Ilya Biryukov via cfe-commits 
Hi!
I had to revert the fix (r348203) in r348280 as this broke our integrate.
Also reverted the original change to avoid having compiler-rt in a broken
state.
Sorry for the inconvenience, see the r348203 thread for more details.


On Tue, Dec 4, 2018 at 12:14 AM Adrian Prantl via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Should be fixed in r348211.
>
> -- adrian
>
> > On Dec 3, 2018, at 3:07 PM, Adrian Prantl via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
> >
> > No, your failures are Windows-specific (/ vs \), and I haven't fixed
> them yet. Thanks for letting me know!
> >
> > -- adrian
> >
> >> On Dec 3, 2018, at 3:06 PM, Galina Kistanova 
> wrote:
> >>
> >> Adrian, did not see your response, please ignore my email.
> >>
> >> Thanks
> >>
> >> Galina
> >>
> >> On Mon, Dec 3, 2018 at 3:04 PM Galina Kistanova 
> wrote:
> >> Hello Adrian,
> >>
> >> This commit broke tests on couple of our builders:
> >>
> >>
> http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/14371
> >>
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast
> >>
> >> . . .
> >> Failing Tests (2):
> >> Clang :: CodeGen/debug-prefix-map.c
> >> Clang :: Modules/module-debuginfo-prefix.m
> >>
> >> The builders were already red and no notifications were sent on this.
> >> Please have a look?
> >>
> >> Thanks
> >>
> >> Galina
> >>
> >> On Mon, Dec 3, 2018 at 9:58 AM Adrian Prantl via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
> >> Author: adrian
> >> Date: Mon Dec  3 09:55:27 2018
> >> New Revision: 348154
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=348154=rev
> >> Log:
> >> Avoid emitting redundant or unusable directories in DIFile metadata
> entries.
> >>
> >> As discussed on llvm-dev recently, Clang currently emits redundant
> >> directories in DIFile entries, such as
> >>
> >>   .file  1 "/Volumes/Data/llvm"
> "/Volumes/Data/llvm/tools/clang/test/CodeGen/debug-info-abspath.c"
> >>
> >> This patch looks at any common prefix between the compilation
> >> directory and the (absolute) file path and strips the redundant
> >> part. More importantly it leaves the compilation directory empty if
> >> the two paths have no common prefix.
> >>
> >> After this patch the above entry is (assuming a compilation dir of
> "/Volumes/Data/llvm/_build"):
> >>
> >>   .file 1 "/Volumes/Data/llvm"
> "tools/clang/test/CodeGen/debug-info-abspath.c"
> >>
> >> When building the FileCheck binary with debug info, this patch makes
> >> the build artifacts ~1kb smaller.
> >>
> >> Differential Revision: https://reviews.llvm.org/D55085
> >>
> >> Added:
> >> cfe/trunk/test/CodeGen/debug-info-abspath.c
> >> Modified:
> >> cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
> >> cfe/trunk/lib/CodeGen/CodeGenAction.cpp
> >> cfe/trunk/test/CodeGen/debug-prefix-map.c
> >> cfe/trunk/test/Modules/module-debuginfo-prefix.m
> >>
> >> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
> >> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=348154=348153=348154=diff
> >>
> ==
> >> --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
> >> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Mon Dec  3 09:55:27 2018
> >> @@ -181,8 +181,7 @@ void CGDebugInfo::setLocation(SourceLoca
> >>SourceManager  = CGM.getContext().getSourceManager();
> >>auto *Scope = cast(LexicalBlockStack.back());
> >>PresumedLoc PCLoc = SM.getPresumedLoc(CurLoc);
> >> -
> >> -  if (PCLoc.isInvalid() || Scope->getFilename() == PCLoc.getFilename())
> >> +  if (PCLoc.isInvalid() || Scope->getFile() == getOrCreateFile(CurLoc))
> >>  return;
> >>
> >>if (auto *LBF = dyn_cast(Scope)) {
> >> @@ -410,13 +409,13 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
> >>SourceManager  = CGM.getContext().getSourceManager();
> >>PresumedLoc PLoc = SM.getPresumedLoc(Loc);
> >>
> >> -  if (PLoc.isInvalid() || StringRef(PLoc.getFilename()).empty())
> >> +  StringRef FileName = PLoc.getFilename();
> >> +  if (PLoc.isInvalid() || FileName.empty())
> >>  // If the location is not valid then use main input file.
> >>  return getOrCreateMainFile();
> >>
> >>// Cache the results.
> >> -  const char *fname = PLoc.getFilename();
> >> -  auto It = DIFileCache.find(fname);
> >> +  auto It = DIFileCache.find(FileName.data());
> >>
> >>if (It != DIFileCache.end()) {
> >>  // Verify that the information still exists.
> >> @@ -431,11 +430,41 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
> >>if (CSKind)
> >>  CSInfo.emplace(*CSKind, Checksum);
> >>
> >> -  llvm::DIFile *F = DBuilder.createFile(
> >> -  remapDIPath(PLoc.getFilename()),
> remapDIPath(getCurrentDirname()), CSInfo,
> >> -  getSource(SM, SM.getFileID(Loc)));
> >> +  StringRef Dir;
> >> +  StringRef File;
> >> +  std::string RemappedFile = remapDIPath(FileName);
> >> +  std::string CurDir = remapDIPath(getCurrentDirname());
>

Re: r348154 - Avoid emitting redundant or unusable directories in DIFile metadata entries.

2018-12-03 Thread Adrian Prantl via cfe-commits 
Should be fixed in r348211.

-- adrian

> On Dec 3, 2018, at 3:07 PM, Adrian Prantl via cfe-commits 
>  wrote:
> 
> No, your failures are Windows-specific (/ vs \), and I haven't fixed them 
> yet. Thanks for letting me know!
> 
> -- adrian
> 
>> On Dec 3, 2018, at 3:06 PM, Galina Kistanova  wrote:
>> 
>> Adrian, did not see your response, please ignore my email.
>> 
>> Thanks
>> 
>> Galina
>> 
>> On Mon, Dec 3, 2018 at 3:04 PM Galina Kistanova  wrote:
>> Hello Adrian,
>> 
>> This commit broke tests on couple of our builders:
>> 
>> http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/14371
>> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast
>> 
>> . . .
>> Failing Tests (2):
>> Clang :: CodeGen/debug-prefix-map.c
>> Clang :: Modules/module-debuginfo-prefix.m
>> 
>> The builders were already red and no notifications were sent on this.
>> Please have a look?
>> 
>> Thanks
>> 
>> Galina
>> 
>> On Mon, Dec 3, 2018 at 9:58 AM Adrian Prantl via cfe-commits 
>>  wrote:
>> Author: adrian
>> Date: Mon Dec  3 09:55:27 2018
>> New Revision: 348154
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=348154=rev
>> Log:
>> Avoid emitting redundant or unusable directories in DIFile metadata entries.
>> 
>> As discussed on llvm-dev recently, Clang currently emits redundant
>> directories in DIFile entries, such as
>> 
>>   .file  1 "/Volumes/Data/llvm" 
>> "/Volumes/Data/llvm/tools/clang/test/CodeGen/debug-info-abspath.c"
>> 
>> This patch looks at any common prefix between the compilation
>> directory and the (absolute) file path and strips the redundant
>> part. More importantly it leaves the compilation directory empty if
>> the two paths have no common prefix.
>> 
>> After this patch the above entry is (assuming a compilation dir of 
>> "/Volumes/Data/llvm/_build"):
>> 
>>   .file 1 "/Volumes/Data/llvm" 
>> "tools/clang/test/CodeGen/debug-info-abspath.c"
>> 
>> When building the FileCheck binary with debug info, this patch makes
>> the build artifacts ~1kb smaller.
>> 
>> Differential Revision: https://reviews.llvm.org/D55085
>> 
>> Added:
>> cfe/trunk/test/CodeGen/debug-info-abspath.c
>> Modified:
>> cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
>> cfe/trunk/lib/CodeGen/CodeGenAction.cpp
>> cfe/trunk/test/CodeGen/debug-prefix-map.c
>> cfe/trunk/test/Modules/module-debuginfo-prefix.m
>> 
>> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=348154=348153=348154=diff
>> ==
>> --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Mon Dec  3 09:55:27 2018
>> @@ -181,8 +181,7 @@ void CGDebugInfo::setLocation(SourceLoca
>>SourceManager  = CGM.getContext().getSourceManager();
>>auto *Scope = cast(LexicalBlockStack.back());
>>PresumedLoc PCLoc = SM.getPresumedLoc(CurLoc);
>> -
>> -  if (PCLoc.isInvalid() || Scope->getFilename() == PCLoc.getFilename())
>> +  if (PCLoc.isInvalid() || Scope->getFile() == getOrCreateFile(CurLoc))
>>  return;
>> 
>>if (auto *LBF = dyn_cast(Scope)) {
>> @@ -410,13 +409,13 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>>SourceManager  = CGM.getContext().getSourceManager();
>>PresumedLoc PLoc = SM.getPresumedLoc(Loc);
>> 
>> -  if (PLoc.isInvalid() || StringRef(PLoc.getFilename()).empty())
>> +  StringRef FileName = PLoc.getFilename();
>> +  if (PLoc.isInvalid() || FileName.empty())
>>  // If the location is not valid then use main input file.
>>  return getOrCreateMainFile();
>> 
>>// Cache the results.
>> -  const char *fname = PLoc.getFilename();
>> -  auto It = DIFileCache.find(fname);
>> +  auto It = DIFileCache.find(FileName.data());
>> 
>>if (It != DIFileCache.end()) {
>>  // Verify that the information still exists.
>> @@ -431,11 +430,41 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>>if (CSKind)
>>  CSInfo.emplace(*CSKind, Checksum);
>> 
>> -  llvm::DIFile *F = DBuilder.createFile(
>> -  remapDIPath(PLoc.getFilename()), remapDIPath(getCurrentDirname()), 
>> CSInfo,
>> -  getSource(SM, SM.getFileID(Loc)));
>> +  StringRef Dir;
>> +  StringRef File;
>> +  std::string RemappedFile = remapDIPath(FileName);
>> +  std::string CurDir = remapDIPath(getCurrentDirname());
>> +  SmallString<128> DirBuf;
>> +  SmallString<128> FileBuf;
>> +  if (llvm::sys::path::is_absolute(RemappedFile)) {
>> +// Strip the common prefix (if it is more than just "/") from current
>> +// directory and FileName for a more space-efficient encoding.
>> +auto FileIt = llvm::sys::path::begin(RemappedFile);
>> +auto FileE = llvm::sys::path::end(RemappedFile);
>> +auto CurDirIt = llvm::sys::path::begin(CurDir);
>> +auto CurDirE = llvm::sys::path::end(CurDir);
>> +for (; CurDirIt != CurDirE && *CurDirIt == *FileIt; ++CurDirIt, 
>> ++FileIt)
>> +

Re: r348154 - Avoid emitting redundant or unusable directories in DIFile metadata entries.

2018-12-03 Thread Adrian Prantl via cfe-commits 
No, your failures are Windows-specific (/ vs \), and I haven't fixed them yet. 
Thanks for letting me know!

-- adrian

> On Dec 3, 2018, at 3:06 PM, Galina Kistanova  wrote:
> 
> Adrian, did not see your response, please ignore my email.
> 
> Thanks
> 
> Galina
> 
> On Mon, Dec 3, 2018 at 3:04 PM Galina Kistanova  > wrote:
> Hello Adrian,
> 
> This commit broke tests on couple of our builders:
> 
> http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/14371
>  
> 
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast
>  
> 
> 
> . . .
> Failing Tests (2):
> Clang :: CodeGen/debug-prefix-map.c
> Clang :: Modules/module-debuginfo-prefix.m
> 
> The builders were already red and no notifications were sent on this.
> Please have a look?
> 
> Thanks
> 
> Galina
> 
> On Mon, Dec 3, 2018 at 9:58 AM Adrian Prantl via cfe-commits 
> mailto:cfe-commits@lists.llvm.org>> wrote:
> Author: adrian
> Date: Mon Dec  3 09:55:27 2018
> New Revision: 348154
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=348154=rev 
> 
> Log:
> Avoid emitting redundant or unusable directories in DIFile metadata entries.
> 
> As discussed on llvm-dev recently, Clang currently emits redundant
> directories in DIFile entries, such as
> 
>   .file  1 "/Volumes/Data/llvm" 
> "/Volumes/Data/llvm/tools/clang/test/CodeGen/debug-info-abspath.c"
> 
> This patch looks at any common prefix between the compilation
> directory and the (absolute) file path and strips the redundant
> part. More importantly it leaves the compilation directory empty if
> the two paths have no common prefix.
> 
> After this patch the above entry is (assuming a compilation dir of 
> "/Volumes/Data/llvm/_build"):
> 
>   .file 1 "/Volumes/Data/llvm" "tools/clang/test/CodeGen/debug-info-abspath.c"
> 
> When building the FileCheck binary with debug info, this patch makes
> the build artifacts ~1kb smaller.
> 
> Differential Revision: https://reviews.llvm.org/D55085 
> 
> 
> Added:
> cfe/trunk/test/CodeGen/debug-info-abspath.c
> Modified:
> cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
> cfe/trunk/lib/CodeGen/CodeGenAction.cpp
> cfe/trunk/test/CodeGen/debug-prefix-map.c
> cfe/trunk/test/Modules/module-debuginfo-prefix.m
> 
> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=348154=348153=348154=diff
>  
> 
> ==
> --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Mon Dec  3 09:55:27 2018
> @@ -181,8 +181,7 @@ void CGDebugInfo::setLocation(SourceLoca
>SourceManager  = CGM.getContext().getSourceManager();
>auto *Scope = cast(LexicalBlockStack.back());
>PresumedLoc PCLoc = SM.getPresumedLoc(CurLoc);
> -
> -  if (PCLoc.isInvalid() || Scope->getFilename() == PCLoc.getFilename())
> +  if (PCLoc.isInvalid() || Scope->getFile() == getOrCreateFile(CurLoc))
>  return;
> 
>if (auto *LBF = dyn_cast(Scope)) {
> @@ -410,13 +409,13 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>SourceManager  = CGM.getContext().getSourceManager();
>PresumedLoc PLoc = SM.getPresumedLoc(Loc);
> 
> -  if (PLoc.isInvalid() || StringRef(PLoc.getFilename()).empty())
> +  StringRef FileName = PLoc.getFilename();
> +  if (PLoc.isInvalid() || FileName.empty())
>  // If the location is not valid then use main input file.
>  return getOrCreateMainFile();
> 
>// Cache the results.
> -  const char *fname = PLoc.getFilename();
> -  auto It = DIFileCache.find(fname);
> +  auto It = DIFileCache.find(FileName.data());
> 
>if (It != DIFileCache.end()) {
>  // Verify that the information still exists.
> @@ -431,11 +430,41 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>if (CSKind)
>  CSInfo.emplace(*CSKind, Checksum);
> 
> -  llvm::DIFile *F = DBuilder.createFile(
> -  remapDIPath(PLoc.getFilename()), remapDIPath(getCurrentDirname()), 
> CSInfo,
> -  getSource(SM, SM.getFileID(Loc)));
> +  StringRef Dir;
> +  StringRef File;
> +  std::string RemappedFile = remapDIPath(FileName);
> +  std::string CurDir = remapDIPath(getCurrentDirname());
> +  SmallString<128> DirBuf;
> +  SmallString<128> FileBuf;
> +  if (llvm::sys::path::is_absolute(RemappedFile)) {
> +// Strip the common prefix (if it is more than just "/") from current
> +// directory and FileName for a more space-efficient encoding.
> +auto FileIt = llvm::sys::path::begin(RemappedFile);
> +auto FileE =

Re: r348154 - Avoid emitting redundant or unusable directories in DIFile metadata entries.

2018-12-03 Thread Galina Kistanova via cfe-commits 
Adrian, did not see your response, please ignore my email.

Thanks

Galina

On Mon, Dec 3, 2018 at 3:04 PM Galina Kistanova 
wrote:

> Hello Adrian,
>
> This commit broke tests on couple of our builders:
>
>
> http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/14371
>
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast
>
> . . .
> Failing Tests (2):
> Clang :: CodeGen/debug-prefix-map.c
> Clang :: Modules/module-debuginfo-prefix.m
>
> The builders were already red and no notifications were sent on this.
> Please have a look?
>
> Thanks
>
> Galina
>
> On Mon, Dec 3, 2018 at 9:58 AM Adrian Prantl via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: adrian
>> Date: Mon Dec  3 09:55:27 2018
>> New Revision: 348154
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=348154=rev
>> Log:
>> Avoid emitting redundant or unusable directories in DIFile metadata
>> entries.
>>
>> As discussed on llvm-dev recently, Clang currently emits redundant
>> directories in DIFile entries, such as
>>
>>   .file  1 "/Volumes/Data/llvm"
>> "/Volumes/Data/llvm/tools/clang/test/CodeGen/debug-info-abspath.c"
>>
>> This patch looks at any common prefix between the compilation
>> directory and the (absolute) file path and strips the redundant
>> part. More importantly it leaves the compilation directory empty if
>> the two paths have no common prefix.
>>
>> After this patch the above entry is (assuming a compilation dir of
>> "/Volumes/Data/llvm/_build"):
>>
>>   .file 1 "/Volumes/Data/llvm"
>> "tools/clang/test/CodeGen/debug-info-abspath.c"
>>
>> When building the FileCheck binary with debug info, this patch makes
>> the build artifacts ~1kb smaller.
>>
>> Differential Revision: https://reviews.llvm.org/D55085
>>
>> Added:
>> cfe/trunk/test/CodeGen/debug-info-abspath.c
>> Modified:
>> cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
>> cfe/trunk/lib/CodeGen/CodeGenAction.cpp
>> cfe/trunk/test/CodeGen/debug-prefix-map.c
>> cfe/trunk/test/Modules/module-debuginfo-prefix.m
>>
>> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=348154=348153=348154=diff
>>
>> ==
>> --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Mon Dec  3 09:55:27 2018
>> @@ -181,8 +181,7 @@ void CGDebugInfo::setLocation(SourceLoca
>>SourceManager  = CGM.getContext().getSourceManager();
>>auto *Scope = cast(LexicalBlockStack.back());
>>PresumedLoc PCLoc = SM.getPresumedLoc(CurLoc);
>> -
>> -  if (PCLoc.isInvalid() || Scope->getFilename() == PCLoc.getFilename())
>> +  if (PCLoc.isInvalid() || Scope->getFile() == getOrCreateFile(CurLoc))
>>  return;
>>
>>if (auto *LBF = dyn_cast(Scope)) {
>> @@ -410,13 +409,13 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>>SourceManager  = CGM.getContext().getSourceManager();
>>PresumedLoc PLoc = SM.getPresumedLoc(Loc);
>>
>> -  if (PLoc.isInvalid() || StringRef(PLoc.getFilename()).empty())
>> +  StringRef FileName = PLoc.getFilename();
>> +  if (PLoc.isInvalid() || FileName.empty())
>>  // If the location is not valid then use main input file.
>>  return getOrCreateMainFile();
>>
>>// Cache the results.
>> -  const char *fname = PLoc.getFilename();
>> -  auto It = DIFileCache.find(fname);
>> +  auto It = DIFileCache.find(FileName.data());
>>
>>if (It != DIFileCache.end()) {
>>  // Verify that the information still exists.
>> @@ -431,11 +430,41 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>>if (CSKind)
>>  CSInfo.emplace(*CSKind, Checksum);
>>
>> -  llvm::DIFile *F = DBuilder.createFile(
>> -  remapDIPath(PLoc.getFilename()), remapDIPath(getCurrentDirname()),
>> CSInfo,
>> -  getSource(SM, SM.getFileID(Loc)));
>> +  StringRef Dir;
>> +  StringRef File;
>> +  std::string RemappedFile = remapDIPath(FileName);
>> +  std::string CurDir = remapDIPath(getCurrentDirname());
>> +  SmallString<128> DirBuf;
>> +  SmallString<128> FileBuf;
>> +  if (llvm::sys::path::is_absolute(RemappedFile)) {
>> +// Strip the common prefix (if it is more than just "/") from current
>> +// directory and FileName for a more space-efficient encoding.
>> +auto FileIt = llvm::sys::path::begin(RemappedFile);
>> +auto FileE = llvm::sys::path::end(RemappedFile);
>> +auto CurDirIt = llvm::sys::path::begin(CurDir);
>> +auto CurDirE = llvm::sys::path::end(CurDir);
>> +for (; CurDirIt != CurDirE && *CurDirIt == *FileIt; ++CurDirIt,
>> ++FileIt)
>> +  llvm::sys::path::append(DirBuf, *CurDirIt);
>> +if (std::distance(llvm::sys::path::begin(CurDir), CurDirIt) == 1) {
>> +  // The common prefix only the root; stripping it would cause
>> +  // LLVM diagnostic locations to be more confusing.
>> +  Dir = {};
>> +  File = RemappedFile;
>> +} else {
>> +  for

Re: r348154 - Avoid emitting redundant or unusable directories in DIFile metadata entries.

2018-12-03 Thread Galina Kistanova via cfe-commits 
Hello Adrian,

This commit broke tests on couple of our builders:

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/14371
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast

. . .
Failing Tests (2):
Clang :: CodeGen/debug-prefix-map.c
Clang :: Modules/module-debuginfo-prefix.m

The builders were already red and no notifications were sent on this.
Please have a look?

Thanks

Galina

On Mon, Dec 3, 2018 at 9:58 AM Adrian Prantl via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: adrian
> Date: Mon Dec  3 09:55:27 2018
> New Revision: 348154
>
> URL: http://llvm.org/viewvc/llvm-project?rev=348154=rev
> Log:
> Avoid emitting redundant or unusable directories in DIFile metadata
> entries.
>
> As discussed on llvm-dev recently, Clang currently emits redundant
> directories in DIFile entries, such as
>
>   .file  1 "/Volumes/Data/llvm"
> "/Volumes/Data/llvm/tools/clang/test/CodeGen/debug-info-abspath.c"
>
> This patch looks at any common prefix between the compilation
> directory and the (absolute) file path and strips the redundant
> part. More importantly it leaves the compilation directory empty if
> the two paths have no common prefix.
>
> After this patch the above entry is (assuming a compilation dir of
> "/Volumes/Data/llvm/_build"):
>
>   .file 1 "/Volumes/Data/llvm"
> "tools/clang/test/CodeGen/debug-info-abspath.c"
>
> When building the FileCheck binary with debug info, this patch makes
> the build artifacts ~1kb smaller.
>
> Differential Revision: https://reviews.llvm.org/D55085
>
> Added:
> cfe/trunk/test/CodeGen/debug-info-abspath.c
> Modified:
> cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
> cfe/trunk/lib/CodeGen/CodeGenAction.cpp
> cfe/trunk/test/CodeGen/debug-prefix-map.c
> cfe/trunk/test/Modules/module-debuginfo-prefix.m
>
> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=348154=348153=348154=diff
>
> ==
> --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Mon Dec  3 09:55:27 2018
> @@ -181,8 +181,7 @@ void CGDebugInfo::setLocation(SourceLoca
>SourceManager  = CGM.getContext().getSourceManager();
>auto *Scope = cast(LexicalBlockStack.back());
>PresumedLoc PCLoc = SM.getPresumedLoc(CurLoc);
> -
> -  if (PCLoc.isInvalid() || Scope->getFilename() == PCLoc.getFilename())
> +  if (PCLoc.isInvalid() || Scope->getFile() == getOrCreateFile(CurLoc))
>  return;
>
>if (auto *LBF = dyn_cast(Scope)) {
> @@ -410,13 +409,13 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>SourceManager  = CGM.getContext().getSourceManager();
>PresumedLoc PLoc = SM.getPresumedLoc(Loc);
>
> -  if (PLoc.isInvalid() || StringRef(PLoc.getFilename()).empty())
> +  StringRef FileName = PLoc.getFilename();
> +  if (PLoc.isInvalid() || FileName.empty())
>  // If the location is not valid then use main input file.
>  return getOrCreateMainFile();
>
>// Cache the results.
> -  const char *fname = PLoc.getFilename();
> -  auto It = DIFileCache.find(fname);
> +  auto It = DIFileCache.find(FileName.data());
>
>if (It != DIFileCache.end()) {
>  // Verify that the information still exists.
> @@ -431,11 +430,41 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>if (CSKind)
>  CSInfo.emplace(*CSKind, Checksum);
>
> -  llvm::DIFile *F = DBuilder.createFile(
> -  remapDIPath(PLoc.getFilename()), remapDIPath(getCurrentDirname()),
> CSInfo,
> -  getSource(SM, SM.getFileID(Loc)));
> +  StringRef Dir;
> +  StringRef File;
> +  std::string RemappedFile = remapDIPath(FileName);
> +  std::string CurDir = remapDIPath(getCurrentDirname());
> +  SmallString<128> DirBuf;
> +  SmallString<128> FileBuf;
> +  if (llvm::sys::path::is_absolute(RemappedFile)) {
> +// Strip the common prefix (if it is more than just "/") from current
> +// directory and FileName for a more space-efficient encoding.
> +auto FileIt = llvm::sys::path::begin(RemappedFile);
> +auto FileE = llvm::sys::path::end(RemappedFile);
> +auto CurDirIt = llvm::sys::path::begin(CurDir);
> +auto CurDirE = llvm::sys::path::end(CurDir);
> +for (; CurDirIt != CurDirE && *CurDirIt == *FileIt; ++CurDirIt,
> ++FileIt)
> +  llvm::sys::path::append(DirBuf, *CurDirIt);
> +if (std::distance(llvm::sys::path::begin(CurDir), CurDirIt) == 1) {
> +  // The common prefix only the root; stripping it would cause
> +  // LLVM diagnostic locations to be more confusing.
> +  Dir = {};
> +  File = RemappedFile;
> +} else {
> +  for (; FileIt != FileE; ++FileIt)
> +llvm::sys::path::append(FileBuf, *FileIt);
> +  Dir = DirBuf;
> +  File = FileBuf;
> +}
> +  } else {
> +Dir = CurDir;
> +File = RemappedFile;
> +  }
> +  llvm::DIFile *F =
> +  DBuilder.createFile(File, Dir,

Re: r348154 - Avoid emitting redundant or unusable directories in DIFile metadata entries.

2018-12-03 Thread Adrian Prantl via cfe-commits 
This should be fixed by LLVM r348203. Thanks for your patience!

-- adrian

> On Dec 3, 2018, at 1:27 PM, Adrian Prantl via cfe-commits 
>  wrote:
> 
> I'll take a look right away. Thanks for letting me know!
> 
> -- adrian
> 
>> On Dec 3, 2018, at 1:26 PM, Vlad Tsyrklevich  wrote:
>> 
>> This change appears to have broken a number of compiler-rt coverage tests, 
>> e.g. in this run. The source of the error appears to be that llvm-cov is now 
>> trying to use a relative path that's not relative to the directory it's in, 
>> though I'm not familiar enough with debuginfo/coverage to understand what an 
>> appropriate fix is. I've not yet reverted these changes to give you a chance 
>> to take a look.
>> 
>> On Mon, Dec 3, 2018 at 9:58 AM Adrian Prantl via cfe-commits 
>>  wrote:
>> Author: adrian
>> Date: Mon Dec  3 09:55:27 2018
>> New Revision: 348154
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=348154=rev
>> Log:
>> Avoid emitting redundant or unusable directories in DIFile metadata entries.
>> 
>> As discussed on llvm-dev recently, Clang currently emits redundant
>> directories in DIFile entries, such as
>> 
>>   .file  1 "/Volumes/Data/llvm" 
>> "/Volumes/Data/llvm/tools/clang/test/CodeGen/debug-info-abspath.c"
>> 
>> This patch looks at any common prefix between the compilation
>> directory and the (absolute) file path and strips the redundant
>> part. More importantly it leaves the compilation directory empty if
>> the two paths have no common prefix.
>> 
>> After this patch the above entry is (assuming a compilation dir of 
>> "/Volumes/Data/llvm/_build"):
>> 
>>   .file 1 "/Volumes/Data/llvm" 
>> "tools/clang/test/CodeGen/debug-info-abspath.c"
>> 
>> When building the FileCheck binary with debug info, this patch makes
>> the build artifacts ~1kb smaller.
>> 
>> Differential Revision: https://reviews.llvm.org/D55085
>> 
>> Added:
>> cfe/trunk/test/CodeGen/debug-info-abspath.c
>> Modified:
>> cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
>> cfe/trunk/lib/CodeGen/CodeGenAction.cpp
>> cfe/trunk/test/CodeGen/debug-prefix-map.c
>> cfe/trunk/test/Modules/module-debuginfo-prefix.m
>> 
>> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=348154=348153=348154=diff
>> ==
>> --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
>> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Mon Dec  3 09:55:27 2018
>> @@ -181,8 +181,7 @@ void CGDebugInfo::setLocation(SourceLoca
>>SourceManager  = CGM.getContext().getSourceManager();
>>auto *Scope = cast(LexicalBlockStack.back());
>>PresumedLoc PCLoc = SM.getPresumedLoc(CurLoc);
>> -
>> -  if (PCLoc.isInvalid() || Scope->getFilename() == PCLoc.getFilename())
>> +  if (PCLoc.isInvalid() || Scope->getFile() == getOrCreateFile(CurLoc))
>>  return;
>> 
>>if (auto *LBF = dyn_cast(Scope)) {
>> @@ -410,13 +409,13 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>>SourceManager  = CGM.getContext().getSourceManager();
>>PresumedLoc PLoc = SM.getPresumedLoc(Loc);
>> 
>> -  if (PLoc.isInvalid() || StringRef(PLoc.getFilename()).empty())
>> +  StringRef FileName = PLoc.getFilename();
>> +  if (PLoc.isInvalid() || FileName.empty())
>>  // If the location is not valid then use main input file.
>>  return getOrCreateMainFile();
>> 
>>// Cache the results.
>> -  const char *fname = PLoc.getFilename();
>> -  auto It = DIFileCache.find(fname);
>> +  auto It = DIFileCache.find(FileName.data());
>> 
>>if (It != DIFileCache.end()) {
>>  // Verify that the information still exists.
>> @@ -431,11 +430,41 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>>if (CSKind)
>>  CSInfo.emplace(*CSKind, Checksum);
>> 
>> -  llvm::DIFile *F = DBuilder.createFile(
>> -  remapDIPath(PLoc.getFilename()), remapDIPath(getCurrentDirname()), 
>> CSInfo,
>> -  getSource(SM, SM.getFileID(Loc)));
>> +  StringRef Dir;
>> +  StringRef File;
>> +  std::string RemappedFile = remapDIPath(FileName);
>> +  std::string CurDir = remapDIPath(getCurrentDirname());
>> +  SmallString<128> DirBuf;
>> +  SmallString<128> FileBuf;
>> +  if (llvm::sys::path::is_absolute(RemappedFile)) {
>> +// Strip the common prefix (if it is more than just "/") from current
>> +// directory and FileName for a more space-efficient encoding.
>> +auto FileIt = llvm::sys::path::begin(RemappedFile);
>> +auto FileE = llvm::sys::path::end(RemappedFile);
>> +auto CurDirIt = llvm::sys::path::begin(CurDir);
>> +auto CurDirE = llvm::sys::path::end(CurDir);
>> +for (; CurDirIt != CurDirE && *CurDirIt == *FileIt; ++CurDirIt, 
>> ++FileIt)
>> +  llvm::sys::path::append(DirBuf, *CurDirIt);
>> +if (std::distance(llvm::sys::path::begin(CurDir), CurDirIt) == 1) {
>> +  // The common prefix only the root; stripping it would cause
>> +  // LLVM diagnostic locations to be more confusing.
>> +

Re: r348154 - Avoid emitting redundant or unusable directories in DIFile metadata entries.

2018-12-03 Thread Adrian Prantl via cfe-commits 
I'll take a look right away. Thanks for letting me know!

-- adrian

> On Dec 3, 2018, at 1:26 PM, Vlad Tsyrklevich  wrote:
> 
> This change appears to have broken a number of compiler-rt coverage tests, 
> e.g. in this run 
> .
>  The source of the error appears to be that llvm-cov is now trying to use a 
> relative path that's not relative to the directory it's in, though I'm not 
> familiar enough with debuginfo/coverage to understand what an appropriate fix 
> is. I've not yet reverted these changes to give you a chance to take a look.
> 
> On Mon, Dec 3, 2018 at 9:58 AM Adrian Prantl via cfe-commits 
> mailto:cfe-commits@lists.llvm.org>> wrote:
> Author: adrian
> Date: Mon Dec  3 09:55:27 2018
> New Revision: 348154
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=348154=rev 
> 
> Log:
> Avoid emitting redundant or unusable directories in DIFile metadata entries.
> 
> As discussed on llvm-dev recently, Clang currently emits redundant
> directories in DIFile entries, such as
> 
>   .file  1 "/Volumes/Data/llvm" 
> "/Volumes/Data/llvm/tools/clang/test/CodeGen/debug-info-abspath.c"
> 
> This patch looks at any common prefix between the compilation
> directory and the (absolute) file path and strips the redundant
> part. More importantly it leaves the compilation directory empty if
> the two paths have no common prefix.
> 
> After this patch the above entry is (assuming a compilation dir of 
> "/Volumes/Data/llvm/_build"):
> 
>   .file 1 "/Volumes/Data/llvm" "tools/clang/test/CodeGen/debug-info-abspath.c"
> 
> When building the FileCheck binary with debug info, this patch makes
> the build artifacts ~1kb smaller.
> 
> Differential Revision: https://reviews.llvm.org/D55085 
> 
> 
> Added:
> cfe/trunk/test/CodeGen/debug-info-abspath.c
> Modified:
> cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
> cfe/trunk/lib/CodeGen/CodeGenAction.cpp
> cfe/trunk/test/CodeGen/debug-prefix-map.c
> cfe/trunk/test/Modules/module-debuginfo-prefix.m
> 
> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=348154=348153=348154=diff
>  
> 
> ==
> --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Mon Dec  3 09:55:27 2018
> @@ -181,8 +181,7 @@ void CGDebugInfo::setLocation(SourceLoca
>SourceManager  = CGM.getContext().getSourceManager();
>auto *Scope = cast(LexicalBlockStack.back());
>PresumedLoc PCLoc = SM.getPresumedLoc(CurLoc);
> -
> -  if (PCLoc.isInvalid() || Scope->getFilename() == PCLoc.getFilename())
> +  if (PCLoc.isInvalid() || Scope->getFile() == getOrCreateFile(CurLoc))
>  return;
> 
>if (auto *LBF = dyn_cast(Scope)) {
> @@ -410,13 +409,13 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>SourceManager  = CGM.getContext().getSourceManager();
>PresumedLoc PLoc = SM.getPresumedLoc(Loc);
> 
> -  if (PLoc.isInvalid() || StringRef(PLoc.getFilename()).empty())
> +  StringRef FileName = PLoc.getFilename();
> +  if (PLoc.isInvalid() || FileName.empty())
>  // If the location is not valid then use main input file.
>  return getOrCreateMainFile();
> 
>// Cache the results.
> -  const char *fname = PLoc.getFilename();
> -  auto It = DIFileCache.find(fname);
> +  auto It = DIFileCache.find(FileName.data());
> 
>if (It != DIFileCache.end()) {
>  // Verify that the information still exists.
> @@ -431,11 +430,41 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>if (CSKind)
>  CSInfo.emplace(*CSKind, Checksum);
> 
> -  llvm::DIFile *F = DBuilder.createFile(
> -  remapDIPath(PLoc.getFilename()), remapDIPath(getCurrentDirname()), 
> CSInfo,
> -  getSource(SM, SM.getFileID(Loc)));
> +  StringRef Dir;
> +  StringRef File;
> +  std::string RemappedFile = remapDIPath(FileName);
> +  std::string CurDir = remapDIPath(getCurrentDirname());
> +  SmallString<128> DirBuf;
> +  SmallString<128> FileBuf;
> +  if (llvm::sys::path::is_absolute(RemappedFile)) {
> +// Strip the common prefix (if it is more than just "/") from current
> +// directory and FileName for a more space-efficient encoding.
> +auto FileIt = llvm::sys::path::begin(RemappedFile);
> +auto FileE = llvm::sys::path::end(RemappedFile);
> +auto CurDirIt = llvm::sys::path::begin(CurDir);
> +auto CurDirE = llvm::sys::path::end(CurDir);
> +for (; CurDirIt != CurDirE && *CurDirIt == *FileIt; ++CurDirIt, ++FileIt)
> +  llvm::sys::path::append(DirBuf, *CurDirIt);
> +if (std::distance(llvm::sys::path::begin(CurDir), CurDirIt) == 1) {
> +  // The common prefix only the root;

Re: r348154 - Avoid emitting redundant or unusable directories in DIFile metadata entries.

2018-12-03 Thread Vlad Tsyrklevich via cfe-commits 
This change appears to have broken a number of compiler-rt coverage tests,
e.g. in this run
.
The source of the error appears to be that llvm-cov is now trying to use a
relative path that's not relative to the directory it's in, though I'm not
familiar enough with debuginfo/coverage to understand what an appropriate
fix is. I've not yet reverted these changes to give you a chance to take a
look.

On Mon, Dec 3, 2018 at 9:58 AM Adrian Prantl via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: adrian
> Date: Mon Dec  3 09:55:27 2018
> New Revision: 348154
>
> URL: http://llvm.org/viewvc/llvm-project?rev=348154=rev
> Log:
> Avoid emitting redundant or unusable directories in DIFile metadata
> entries.
>
> As discussed on llvm-dev recently, Clang currently emits redundant
> directories in DIFile entries, such as
>
>   .file  1 "/Volumes/Data/llvm"
> "/Volumes/Data/llvm/tools/clang/test/CodeGen/debug-info-abspath.c"
>
> This patch looks at any common prefix between the compilation
> directory and the (absolute) file path and strips the redundant
> part. More importantly it leaves the compilation directory empty if
> the two paths have no common prefix.
>
> After this patch the above entry is (assuming a compilation dir of
> "/Volumes/Data/llvm/_build"):
>
>   .file 1 "/Volumes/Data/llvm"
> "tools/clang/test/CodeGen/debug-info-abspath.c"
>
> When building the FileCheck binary with debug info, this patch makes
> the build artifacts ~1kb smaller.
>
> Differential Revision: https://reviews.llvm.org/D55085
>
> Added:
> cfe/trunk/test/CodeGen/debug-info-abspath.c
> Modified:
> cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
> cfe/trunk/lib/CodeGen/CodeGenAction.cpp
> cfe/trunk/test/CodeGen/debug-prefix-map.c
> cfe/trunk/test/Modules/module-debuginfo-prefix.m
>
> Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=348154=348153=348154=diff
>
> ==
> --- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Mon Dec  3 09:55:27 2018
> @@ -181,8 +181,7 @@ void CGDebugInfo::setLocation(SourceLoca
>SourceManager  = CGM.getContext().getSourceManager();
>auto *Scope = cast(LexicalBlockStack.back());
>PresumedLoc PCLoc = SM.getPresumedLoc(CurLoc);
> -
> -  if (PCLoc.isInvalid() || Scope->getFilename() == PCLoc.getFilename())
> +  if (PCLoc.isInvalid() || Scope->getFile() == getOrCreateFile(CurLoc))
>  return;
>
>if (auto *LBF = dyn_cast(Scope)) {
> @@ -410,13 +409,13 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>SourceManager  = CGM.getContext().getSourceManager();
>PresumedLoc PLoc = SM.getPresumedLoc(Loc);
>
> -  if (PLoc.isInvalid() || StringRef(PLoc.getFilename()).empty())
> +  StringRef FileName = PLoc.getFilename();
> +  if (PLoc.isInvalid() || FileName.empty())
>  // If the location is not valid then use main input file.
>  return getOrCreateMainFile();
>
>// Cache the results.
> -  const char *fname = PLoc.getFilename();
> -  auto It = DIFileCache.find(fname);
> +  auto It = DIFileCache.find(FileName.data());
>
>if (It != DIFileCache.end()) {
>  // Verify that the information still exists.
> @@ -431,11 +430,41 @@ llvm::DIFile *CGDebugInfo::getOrCreateFi
>if (CSKind)
>  CSInfo.emplace(*CSKind, Checksum);
>
> -  llvm::DIFile *F = DBuilder.createFile(
> -  remapDIPath(PLoc.getFilename()), remapDIPath(getCurrentDirname()),
> CSInfo,
> -  getSource(SM, SM.getFileID(Loc)));
> +  StringRef Dir;
> +  StringRef File;
> +  std::string RemappedFile = remapDIPath(FileName);
> +  std::string CurDir = remapDIPath(getCurrentDirname());
> +  SmallString<128> DirBuf;
> +  SmallString<128> FileBuf;
> +  if (llvm::sys::path::is_absolute(RemappedFile)) {
> +// Strip the common prefix (if it is more than just "/") from current
> +// directory and FileName for a more space-efficient encoding.
> +auto FileIt = llvm::sys::path::begin(RemappedFile);
> +auto FileE = llvm::sys::path::end(RemappedFile);
> +auto CurDirIt = llvm::sys::path::begin(CurDir);
> +auto CurDirE = llvm::sys::path::end(CurDir);
> +for (; CurDirIt != CurDirE && *CurDirIt == *FileIt; ++CurDirIt,
> ++FileIt)
> +  llvm::sys::path::append(DirBuf, *CurDirIt);
> +if (std::distance(llvm::sys::path::begin(CurDir), CurDirIt) == 1) {
> +  // The common prefix only the root; stripping it would cause
> +  // LLVM diagnostic locations to be more confusing.
> +  Dir = {};
> +  File = RemappedFile;
> +} else {
> +  for (; FileIt != FileE; ++FileIt)
> +llvm::sys::path::append(FileBuf, *FileIt);
> +  Dir = DirBuf;
> +  File = FileBuf;
> +}
> +  } else {
> +Dir = CurDir;
> +File = RemappedFile;
> +  }
> +