Fix race condition in Defined Atom ordinal computation.

[lhames]

Summary

Defined atom are allocated in multiples thread. The static counter must be atomic, else the ordinal is not guarantee to be unique (which is required by the Mach-O writer).

[shankarke]

am confused, Atoms live within a File and when the ordinals would be relative inside the file. So for your case do more than one file share the same SimpleDefinedAtom ? Am I missing some information ?

[Jean-Daniel]

That’s true for input file, but having the same ordinal for 2 atoms raise an assert in MachO Layout Pass which is run on the generated file (that contains atom from many files)

Le 14 mars 2015 à 06:22, Shankar Kalpathi Easwaran <[e-mail]> a écrit :

am confused, Atoms live within a File and when the ordinals would be relative inside the file. So for your case do more than one file share the same SimpleDefinedAtom ? Am I missing some information ?

REPOSITORY
rL LLVM

http://reviews.llvm.org/D8336

EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/

[Jean-Daniel]

But now that I think about it, there is just something wrong in the first place with that ordinal. It it either meaning full inside a single input file, and we shouldn’t use a static or don’t care about race condition, or it is meaningful across files and I don’t see how it can be as loading file in parallel make it indeterministic anyway.

Le 14 mars 2015 à 07:22, Jean-Daniel Dupas <[e-mail]> a écrit :

That’s true for input file, but having the same ordinal for 2 atoms raise an assert in MachO Layout Pass which is run on the generated file (that contains atom from many files)

Le 14 mars 2015 à 06:22, Shankar Kalpathi Easwaran <[e-mail]> a écrit :

am confused, Atoms live within a File and when the ordinals would be relative inside the file. So for your case do more than one file share the same SimpleDefinedAtom ? Am I missing some information ?

REPOSITORY
rL LLVM

http://reviews.llvm.org/D8336

EMAIL PREFERENCES
http://reviews.llvm.org/settings/panel/emailpreferences/

---

llvm-commits mailing list
[e-mail]
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

[ruiu]

Thank you for doing this. I wonder if we use _ordinal assigned here in the constructor. We usually assign ordinals explicitly in the reader, so I hope ordinals assigned here are overwritten in all use cases. If you change this to UINT_MAX or something and make ordinal() to abort if it's still UINT_MAX, does the linker fail with the assertion?

This change is good to mitigate the situation, so I'm OK to commit as a quick fix, but the actual fix should be assigning ordinals explicitly.

[lhames]

I just hit this bug too (see http://reviews.llvm.org/D8336), and I've committed this fix in r237857.

Jean-Daniel - did you end up having any insights into the Right Way to fix this?

[lhames]

Fixed in r238072 by moving the ordinal tracking into the File class as discussed in http://reviews.llvm.org/D9927.

include/lld/Core/Simple.h

 };
 }
 
 namespace lld {
 
 class SimpleDefinedAtom : public DefinedAtom {
 public:
   explicit SimpleDefinedAtom(const File &f) : _file(f) {
-    static uint32_t lastOrdinal = 0;
+    static std::atomic<uint32_t> lastOrdinal;
     _ordinal = lastOrdinal++;
     _references.setAllocator(&f.allocator());
   }
 
   const File &file() const override { return _file; }
 
   StringRef name() const override { return StringRef(); }