fixed ambiguous overload build failure

[juchem]

Summary

LLVM (llvmorg-14-init) under Debian sid using latest gcc (Debian 10.3.0-9) 10.3.0 fails due to ambiguous overload on operators == and !=:

/root/src/llvm/src/llvm/tools/obj2yaml/elf2yaml.cpp:212:22:
error: ambiguous overload for 'operator!='
(operand types are 'llvm::ELFYAML::ELF_SHF' and 'int')

/root/src/llvm/src/llvm/tools/obj2yaml/elf2yaml.cpp:204:32:
error: ambiguous overload for 'operator!='
(operand types are 'const llvm::yaml::Hex64' and 'int')

/root/src/llvm/src/llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:629:35:
error: ambiguous overload for 'operator=='
(operand types are 'const uint64_t' {aka 'const long unsigned int'} and 'llvm::Register')

[jmorse]

For the VarLocBasedImpl.cpp change, I'd recommend explicitly constructing a Register from the RegNo field -- it'll achieve exactly the same thing, but avoids the cast and is a bit clearer to the reader.

I'm curious as to why this has cropped up in gcc now; however I'm not a language lawyer.

[StephenTozer]

I'd repeat the above comment for llvm::yaml::Hex64 and llvm::ELFYAML::ELF_SHF, using the constructor is more readable and sound.

[juchem]

switched from static_cast to explicit cast constructor

[juchem]

re-running clang-format

[Higuoxing]

You are able to drop llvm::. Otherwise, LGTM.

[juchem]

It seems there's a CI build failure due to a narrowing implicit cast. I'll fix that and drop the llvm:: name qualifier.

[juchem]

fixing CI failure due to narrowing cast
dropping llvm:: name qualifier

[juchem]

@Higuoxing the test that's failing on CI sounds unrelated to this patch. Can you help me figure out if that's the case? I'm unfamiliar with LLVM CI.
Also, I am not authorized to merge. If everything looks fine, who can help me with that?

[Higuoxing]

In D109534#2999856, @juchem wrote:

@Higuoxing the test that's failing on CI sounds unrelated to this patch. Can you help me figure out if that's the case? I'm unfamiliar with LLVM CI.
Also, I am not authorized to merge. If everything looks fine, who can help me with that?

Yeah, the failure isn't caused by your change. Could you please provide your email and prefered name in the commit message?

[juchem]

@Higuoxing I was under the impression the patch file I've uploaded had that already. How do I add this information through the phabricator UI, since I don't have the command line tools installed?

[Higuoxing]

Hi, @jhenderson, could you please help @juchem push his change since I haven't pushed to LLVM repo for a time.

[jhenderson]

In D109534#3003141, @juchem wrote:

@Higuoxing I was under the impression the patch file I've uploaded had that already. How do I add this information through the phabricator UI, since I don't have the command line tools installed?

The "Download Raw Diff" file doesn't contain any details. I don't know of any way to retrieve details via the UI. For some users, it might be possible to get the info from the uploaded patch, if they use the arcanist tool. However, many people don't use that tool (such as myself). As such, you'll need to post your details directly in this thread, or email them directly to whoever is going to land the patch for you. Usually the person who lands the patch would be someone involved in the review (e.g. @jmorse/@StephenTozer), or someone you work directly with who has commit access. If nobody else is able to, I can commit if needed.

Unrelated to landing the patch, but in the future, please upload diffs with full context (i.e. add something like -U999999), as this makes it easier to review. Also, as the two fixes are not really related (they're in completely different parts of the code base), typically I'd have done this in two different patches, but I'm not suggesting you split them up now.

[juchem]

Thanks @jhenderson, that makes sense.
My email is the username I'm using here at Gmail dot com.
We can use my username as my preferred name too.
I don't work with anyone who has commit access.
@jmorse / @StephenTozer, can any of you take care of the merging? Thanks in advance.

[juchem]

@jhenderson / @jmorse / @StephenTozer Is there anything else I can do to help with landing this?

[jhenderson]

I'll take a look at landing this now. Once the change has landed, you should be automatically notified on this review. Please keep an eye out for build bot failure notifications and try to confirm whether they are caused by yours or a different patch.

[juchem]

thanks @jhenderson
I'll keep an eye out

llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp

     unsigned getRegIdx(Register Reg) const {
       for (unsigned Idx = 0; Idx < Locs.size(); ++Idx)
         if (Locs[Idx].Kind == MachineLocKind::RegisterKind &&
-            Locs[Idx].Value.RegNo == Reg)
+            llvm::Register{Locs[Idx].Value.RegNo} == Reg)
           return Idx;
       llvm_unreachable("Could not find given Reg in Locs");
     }

llvm/tools/obj2yaml/elf2yaml.cpp

     if (const ELFYAML::RawContentSection *RawSec =
             dyn_cast<const ELFYAML::RawContentSection>(&S)) {
       if (RawSec->Type != ELF::SHT_PROGBITS || RawSec->Link || RawSec->Info ||
-          RawSec->AddressAlign != 1 || RawSec->Address || RawSec->EntSize)
+          RawSec->AddressAlign != llvm::yaml::Hex64{1} || RawSec->Address ||
+          RawSec->EntSize)
         return true;
 
       ELFYAML::ELF_SHF ShFlags = RawSec->Flags.getValueOr(ELFYAML::ELF_SHF(0));

       if (SecName == "debug_str")
         return ShFlags != ELFYAML::ELF_SHF(ELF::SHF_MERGE | ELF::SHF_STRINGS);
 
-      return ShFlags != 0;
+      return ShFlags != llvm::ELFYAML::ELF_SHF{0};
     }
   }