Fix DataExtractor symbol conflict

[emrekultursay]

Summary

There are two DataExtractors in scope: one from the llvm namespace
and one from the lldb_private namespace. Some Microsoft Visual C++
compilers (I tested with MSVC 14.23 specifically) cannot handle this
situation, and generate ambiguous symbol errors. This change fixes
this compile error.

[shafik]

LGTM

[emrekultursay]

Hi shafik, can you also submit it? I don't have submit permission. Thanks.

[JDevlieghere]

I think the better solution here is to get rid of the using namespace llvm; in the implementation file instead.

[shafik]

In D120718#3352450, @JDevlieghere wrote:

I think the better solution here is to get rid of the using namespace llvm; in the implementation file instead.

That is a good point. I did a quick look at the file DataFileCache.h and none of the files it includes directly use using namespace llvm;.

It looks like across llvm-project we have 54 headers that use using namespace llvm; I thought it was a lot more.

I personally dislike it even in .cpp files using llvm:: or lldb_private:: does not feel like an undue burden. Although the count for .cpp files is 4000+ 😱

[labath]

In D120718#3352635, @shafik wrote:

In D120718#3352450, @JDevlieghere wrote:

I think the better solution here is to get rid of the using namespace llvm; in the implementation file instead.

That is a good point. I did a quick look at the file DataFileCache.h and none of the files it includes directly use using namespace llvm;.

It looks like across llvm-project we have 54 headers that use using namespace llvm; I thought it was a lot more.

It looks like at least some of those are inside functions, which is kind of OK. But what's worse, some of those are not inside functions, which is 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱. using directives in headers completely mangle the symbol lookups in any file which includes them.

[emrekultursay]

Removed "using namespace llvm" from the implementation file
and qualified all associated references with "llvm::".

[emrekultursay]

In D120718#3352450, @JDevlieghere wrote:

I think the better solution here is to get rid of the using namespace llvm; in the implementation file instead.

Done. PTAL.

[JDevlieghere]

Thanks for cleaning up the implementation file. LGMT module removing the added lldb_private:: which I believe we no longer need.

lldb/include/lldb/Core/DataFileCache.h

164 ↗

(On Diff #412361)

I assume the header was never a problem and you did this to match the signature in the implementation file (which no longer needs the namespace qualifier without the using namespace llvm), unless one of the headers included here (transitively) includes a header that has the "😱 😱 😱" top level using namespace llvm;?

[emrekultursay]

Reverted the changes to the header file.

[emrekultursay]

Thanks. PTAL.

[JDevlieghere]

Thank you! LGTM

lldb/source/Core/DataFileCache.cpp

 #include "lldb/Host/FileSystem.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/DataEncoder.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
 #include "llvm/Support/CachePruning.h"
 #include "llvm/Support/MemoryBuffer.h"
 
-using namespace llvm;
 using namespace lldb_private;
 
-DataFileCache::DataFileCache(StringRef path) {
+DataFileCache::DataFileCache(llvm::StringRef path) {
   m_cache_dir.SetPath(path);
 
   // Prune the cache based off of the LLDB settings each time we create a cache
   // object.
   ModuleListProperties &properties =
       ModuleList::GetGlobalModuleListProperties();
-  CachePruningPolicy policy;
+  llvm::CachePruningPolicy policy;
   // Only scan once an hour. If we have lots of debug sessions we don't want
   // to scan this directory too often. A timestamp file is written to the
   // directory to ensure different processes don't scan the directory too often.
   // This setting doesn't mean that a thread will continually scan the cache
   // directory within this process.
   policy.Interval = std::chrono::hours(1);
   // Get the user settings for pruning.
   policy.MaxSizeBytes = properties.GetLLDBIndexCacheMaxByteSize();
   policy.MaxSizePercentageOfAvailableSpace =
       properties.GetLLDBIndexCacheMaxPercent();
   policy.Expiration =
       std::chrono::hours(properties.GetLLDBIndexCacheExpirationDays() * 24);
   pruneCache(path, policy);
 
   // This lambda will get called when the data is gotten from the cache and
   // also after the data was set for a given key. We only need to take
   // ownership of the data if we are geting the data, so we use the
   // m_take_ownership member variable to indicate if we need to take
   // ownership.
 
   auto add_buffer = [this](unsigned task, std::unique_ptr<llvm::MemoryBuffer> m) {
     if (m_take_ownership)
       m_mem_buff_up = std::move(m);
   };
-  Expected<FileCache> cache_or_err =
+  llvm::Expected<llvm::FileCache> cache_or_err =
       llvm::localCache("LLDBModuleCache", "lldb-module", path, add_buffer);
   if (cache_or_err)
     m_cache_callback = std::move(*cache_or_err);
   else {
     Log *log = GetLog(LLDBLog::Modules);
     LLDB_LOG_ERROR(log, cache_or_err.takeError(),
                    "failed to create lldb index cache directory: {0}");
   }
 }
 
 std::unique_ptr<llvm::MemoryBuffer>
-DataFileCache::GetCachedData(StringRef key) {
+DataFileCache::GetCachedData(llvm::StringRef key) {
   std::lock_guard<std::mutex> guard(m_mutex);
 
   const unsigned task = 1;
   m_take_ownership = true;
   // If we call the "m_cache_callback" function and the data is cached, it will
   // call the "add_buffer" lambda function from the constructor which will in
   // turn take ownership of the member buffer that is passed to the callback and
   // put it into a member variable.
-  Expected<AddStreamFn> add_stream_or_err = m_cache_callback(task, key);
+  llvm::Expected<llvm::AddStreamFn> add_stream_or_err =
+      m_cache_callback(task, key);
   m_take_ownership = false;
   // At this point we either already called the "add_buffer" lambda with
   // the data or we haven't. We can tell if we got the cached data by checking
   // the add_stream function pointer value below.
   if (add_stream_or_err) {
-    AddStreamFn &add_stream = *add_stream_or_err;
+    llvm::AddStreamFn &add_stream = *add_stream_or_err;
     // If the "add_stream" is nullptr, then the data was cached and we already
     // called the "add_buffer" lambda. If it is valid, then if we were to call
     // the add_stream function it would cause a cache file to get generated
     // and we would be expected to fill in the data. In this function we only
     // want to check if the data was cached, so we don't want to call
     // "add_stream" in this function.
     if (!add_stream)
       return std::move(m_mem_buff_up);
   } else {
     Log *log = GetLog(LLDBLog::Modules);
     LLDB_LOG_ERROR(log, add_stream_or_err.takeError(),
                    "failed to get the cache add stream callback for key: {0}");
   }
   // Data was not cached.
   return std::unique_ptr<llvm::MemoryBuffer>();
 }
 
-bool DataFileCache::SetCachedData(StringRef key, llvm::ArrayRef<uint8_t> data) {
+bool DataFileCache::SetCachedData(llvm::StringRef key,
+                                  llvm::ArrayRef<uint8_t> data) {
   std::lock_guard<std::mutex> guard(m_mutex);
   const unsigned task = 2;
   // If we call this function and the data is cached, it will call the
   // add_buffer lambda function from the constructor which will ignore the
   // data.
-  Expected<AddStreamFn> add_stream_or_err = m_cache_callback(task, key);
+  llvm::Expected<llvm::AddStreamFn> add_stream_or_err =
+      m_cache_callback(task, key);
   // If we reach this code then we either already called the callback with
   // the data or we haven't. We can tell if we had the cached data by checking
   // the CacheAddStream function pointer value below.
   if (add_stream_or_err) {
-    AddStreamFn &add_stream = *add_stream_or_err;
+    llvm::AddStreamFn &add_stream = *add_stream_or_err;
     // If the "add_stream" is nullptr, then the data was cached. If it is
     // valid, then if we call the add_stream function with a task it will
     // cause the file to get generated, but we only want to check if the data
     // is cached here, so we don't want to call it here. Note that the
     // add_buffer will also get called in this case after the data has been
     // provided, but we won't take ownership of the memory buffer as we just
     // want to write the data.
     if (add_stream) {
-      Expected<std::unique_ptr<CachedFileStream>> file_or_err =
+      llvm::Expected<std::unique_ptr<llvm::CachedFileStream>> file_or_err =
           add_stream(task);
       if (file_or_err) {
-        CachedFileStream *cfs = file_or_err->get();
+        llvm::CachedFileStream *cfs = file_or_err->get();
         cfs->OS->write((const char *)data.data(), data.size());
         return true;
       } else {
         Log *log = GetLog(LLDBLog::Modules);
         LLDB_LOG_ERROR(log, file_or_err.takeError(),
                        "failed to get the cache file stream for key: {0}");
       }
     }
bool CacheSignature::Encode(DataEncoder &encoder) {
   if (m_obj_mod_time.hasValue()) {
     encoder.AppendU8(eSignatureObjectModTime);
     encoder.AppendU32(*m_obj_mod_time);
   }
   encoder.AppendU8(eSignatureEnd);
   return true;
 }
 
-bool CacheSignature::Decode(const DataExtractor &data,
+bool CacheSignature::Decode(const lldb_private::DataExtractor &data,
                             lldb::offset_t *offset_ptr) {
   Clear();
   while (uint8_t sig_encoding = data.GetU8(offset_ptr)) {
     switch (sig_encoding) {
     case eSignatureUUID: {
       const uint8_t length = data.GetU8(offset_ptr);
       const uint8_t *bytes = (const uint8_t *)data.GetData(offset_ptr, length);
       if (bytes != nullptr && length > 0)
for (auto s: m_strings) {
     // Append the C string into the encoder
     encoder.AppendCString(s.GetStringRef());
   }
   // Fixup the string table length.
   encoder.PutU32(length_offset, encoder.GetByteSize() - strtab_offset);
   return true;
 }
 
-bool StringTableReader::Decode(const DataExtractor &data,
+bool StringTableReader::Decode(const lldb_private::DataExtractor &data,
                                lldb::offset_t *offset_ptr) {
   llvm::StringRef identifier((const char *)data.GetData(offset_ptr, 4), 4);
   if (identifier != kStringTableIdentifier)
     return false;
   const uint32_t length = data.GetU32(offset_ptr);
   // We always have at least one byte for the empty string at offset zero.
   if (length == 0)
     return false;
   const char *bytes = (const char *)data.GetData(offset_ptr, length);
   if (bytes == nullptr)
     return false;
-  m_data = StringRef(bytes, length);
+  m_data = llvm::StringRef(bytes, length);
   return true;
 }
 
-StringRef StringTableReader::Get(uint32_t offset) const {
+llvm::StringRef StringTableReader::Get(uint32_t offset) const {
   if (offset >= m_data.size())
-    return StringRef();
-  return StringRef(m_data.data() + offset);
+    return llvm::StringRef();
+  return llvm::StringRef(m_data.data() + offset);
 }